idaes-pse
idaes-pse copied to clipboard
Add 1D Fixed Bed reactor model to the IDAES gas-solids contactor library
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:
- I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
- 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.
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 The unit consistency problem is a know issue in Pyomo https://github.com/Pyomo/pyomo/issues/1790
@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 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.
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 it appears the files moving_bed.py
and bubbling_fluidized_bed.py
are not Black-formatted.
@chineduobiora First thing to do is make sure you are using a compatible version of black
.
@chineduobiora it appears the files
moving_bed.py
andbubbling_fluidized_bed.py
are not Black-formatted.
That's what it says but I've black-formatted them on my local repo.
@chineduobiora First thing to do is make sure you are using a compatible version of
black
.
Ahh. That might be it. Thanks.
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.
@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 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.