idaes-pse icon indicating copy to clipboard operation
idaes-pse copied to clipboard

Add 1D Fixed Bed reactor model to the IDAES gas-solids contactor library

Open chineduobiora opened this issue 2 years ago • 12 comments

Fixes

Summary/Motivation:

This PR will add a 1D fixed bed (1DFixedBed) unit model to the IDAES gas-solids contactor library. The 1DFixedBed is an axially varying time-dependent model with gas and solid phases modeled in detail.

Changes proposed in this PR:

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

chineduobiora avatar Oct 06 '22 23:10 chineduobiora

test_units_consistent failure is from DAE transformation. Pyomo.DAE does not account for the units of the ContinuousSet (time, space etc) in the transformed DerivativeVar expression thus leading to the InconsistentUnitsError for the accumulation expressions. See example below: pyomo.core.base.units_container.InconsistentUnitsError: Error in units found in expression: fs.unit.gas_phase.material_accumulation[120.0,0.0,Vap,CH4] - 0.008333333333333333*(fs.unit.gas_phase.material_holdup[120.0,0.0,Vap,CH4] - fs.unit.gas_phase.material_holdup[0.0,0.0,Vap,CH4]): mole / meter / second not compatible with mole / meter.

chineduobiora avatar Oct 06 '22 23:10 chineduobiora

@chineduobiora The unit consistency problem is a know issue in Pyomo https://github.com/Pyomo/pyomo/issues/1790

andrewlee94 avatar Oct 07 '22 14:10 andrewlee94

@chineduobiora The unit consistency problem is a know issue in Pyomo Pyomo/pyomo#1790

How do we resolve it for this PR so the tests don't fail?

chineduobiora avatar Oct 07 '22 19:10 chineduobiora

@chineduobiora For now, I would xfail the test - this will flag is as expected to fail and remind us to fix it when Pyomo addresses the issue. You could also add a comment to the Pyomo issue in the hopes of prompting them to fix it sooner.

andrewlee94 avatar Oct 07 '22 19:10 andrewlee94

All review comments have been addressed. @andrewlee94 and @bpaul4 do you have ideas why the "check code formatting (Black)" test is failing. I've run black on my local machine without any issues noted.

chineduobiora avatar Oct 11 '22 19:10 chineduobiora

@chineduobiora it appears the files moving_bed.py and bubbling_fluidized_bed.py are not Black-formatted.

bpaul4 avatar Oct 11 '22 19:10 bpaul4

@chineduobiora First thing to do is make sure you are using a compatible version of black.

andrewlee94 avatar Oct 11 '22 19:10 andrewlee94

@chineduobiora it appears the files moving_bed.py and bubbling_fluidized_bed.py are not Black-formatted.

That's what it says but I've black-formatted them on my local repo.

chineduobiora avatar Oct 11 '22 19:10 chineduobiora

@chineduobiora First thing to do is make sure you are using a compatible version of black.

Ahh. That might be it. Thanks.

chineduobiora avatar Oct 11 '22 19:10 chineduobiora

Codecov Report

Base: 69.95% // Head: 69.97% // Increases project coverage by +0.01% :tada:

Coverage data is based on head (4410c4f) compared to base (52507c7). Patch coverage: 71.38% of modified lines in pull request are covered.

:exclamation: Current head 4410c4f differs from pull request most recent head 086a137. Consider uploading reports for the commit 086a137 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #984      +/-   ##
==========================================
+ Coverage   69.95%   69.97%   +0.01%     
==========================================
  Files         399      398       -1     
  Lines       65505    65591      +86     
  Branches    11984    12070      +86     
==========================================
+ Hits        45827    45896      +69     
- Misses      17367    17377      +10     
- Partials     2311     2318       +7     
Impacted Files Coverage Δ
...d_contactors/unit_models/bubbling_fluidized_bed.py 73.67% <ø> (ø)
...tra/gas_solid_contactors/unit_models/moving_bed.py 65.12% <0.00%> (-0.23%) :arrow_down:
...a/gas_solid_contactors/unit_models/fixed_bed_0D.py 89.26% <50.00%> (-0.54%) :arrow_down:
...a/gas_solid_contactors/unit_models/fixed_bed_1D.py 71.62% <71.62%> (ø)
...extra/gas_solid_contactors/unit_models/__init__.py 100.00% <100.00%> (ø)
idaes/ver.py 61.53% <0.00%> (-4.62%) :arrow_down:
...ra/power_generation/costing/power_plant_costing.py 77.16% <0.00%> (-0.53%) :arrow_down:
...a/power_generation/costing/costing_dictionaries.py 100.00% <0.00%> (ø)
...eration/costing/generic_ccs_capcost_custom_dict.py
...ra/power_generation/costing/power_plant_capcost.py
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar Oct 11 '22 20:10 codecov[bot]

@andrewlee94 @bpaul4 @Daison2102, all review comments have been addressed and all tests pass, let me know if there are any additional comments you have. Also, note that the TSA example (https://github.com/IDAES/examples-pse/pull/152) is also dependent on the approval of this PR.

chineduobiora avatar Oct 13 '22 15:10 chineduobiora

@chineduobiora I have no further comments on the model, everything looks good. If you'd like to test the example before the model is merged, you could temporarily change the workflow head to the pull request branch (see my PR 151 in examples-pse where I did this for a notebook that depends on unmerged code) and then revert it back before merging the example.

@bpaul4 Thanks! I'll try it out.

chineduobiora avatar Oct 13 '22 16:10 chineduobiora