Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 14 commits into from
Oct 18, 2022

Conversation

chineduobiora
Copy link
Collaborator

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 chineduobiora added enhancement New feature or request Priority:High High Priority Issue or PR unit models Issues dealing with the unit model libraries labels Oct 6, 2022
@chineduobiora
Copy link
Collaborator Author

chineduobiora commented Oct 6, 2022

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 chineduobiora requested a review from xiangyuy October 6, 2022 23:37
@andrewlee94
Copy link
Member

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

Copy link
Member

@andrewlee94 andrewlee94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked at the equations in detail (that would probably be best left to someone with more experience in these types of system), but I had some general comments.

Overall, this looks good but it needs to have at least some minimal documentation.

Copy link
Contributor

@bpaul4 bpaul4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The model equations and tests look good, great job @chineduobiora! Everything looks good to me, excluding the known DAE units issue and a minor comment regarding the balance check tests.

@chineduobiora
Copy link
Collaborator Author

@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?

@andrewlee94
Copy link
Member

@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.

chineduobiora added 3 commits October 8, 2022 18:22
@chineduobiora
Copy link
Collaborator Author

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.

@bpaul4
Copy link
Contributor

bpaul4 commented Oct 11, 2022

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

@andrewlee94
Copy link
Member

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

@chineduobiora
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

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

Ahh. That might be it. Thanks.

@codecov
Copy link

codecov bot commented Oct 11, 2022

Codecov Report

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

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

❗ 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%) ⬇️
...a/gas_solid_contactors/unit_models/fixed_bed_0D.py 89.26% <50.00%> (-0.54%) ⬇️
...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%) ⬇️
...ra/power_generation/costing/power_plant_costing.py 77.16% <0.00%> (-0.53%) ⬇️
...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.

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

@chineduobiora
Copy link
Collaborator Author

@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 (IDAES/examples-pse#152) is also dependent on the approval of this PR.

Copy link
Contributor

@bpaul4 bpaul4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@chineduobiora
Copy link
Collaborator Author

@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.

@@ -216,32 +228,18 @@ class FixedBed1DData(UnitModelBlockData):
"pressure_drop_type",
ConfigValue(
default="ergun_correlation",
domain=In(["simple_correlation", "ergun_correlation"]),
domain=In(["ergun_correlation", "simple_correlation"]),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed now, but for the future you should consider using Python Enums for this - they have the benefit of auto-completion in IDEs and documentation as a starting point.

@andrewlee94 andrewlee94 merged commit 29fd1cd into IDAES:main Oct 18, 2022
@chineduobiora chineduobiora deleted the fixedbed1D branch October 18, 2022 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Priority:High High Priority Issue or PR unit models Issues dealing with the unit model libraries
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

4 participants