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

Clean up precipitator and documentation #62

Merged
merged 20 commits into from
Apr 30, 2024

Conversation

agarciadiego
Copy link
Contributor

Addresses Issue:

Adds documentation to preliminary precipitator model.

Summary/Motivation:

  • Precipitator had no documentation

Changes proposed in this PR:

  • Change cv_precipitate from ControlVolume0DBlock to state_block and need to fix an inexistent inlet for the precipitate phase
  • Adds documentation for unit and property packages

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

@agarciadiego agarciadiego self-assigned this Apr 23, 2024
@agarciadiego agarciadiego marked this pull request as draft April 23, 2024 18:34
@agarciadiego agarciadiego added Priority:Normal Normal Priority Issue or PR documentation Improvements or additions to documentation enhancement New feature or request labels Apr 23, 2024
@agarciadiego agarciadiego mentioned this pull request Apr 26, 2024
12 tasks
Copy link

codecov bot commented Apr 26, 2024

Codecov Report

Attention: Patch coverage is 95.65217% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 95.89%. Comparing base (f4b6794) to head (ffc7575).

Files Patch % Lines
src/prommis/precipitate/precipitator.py 91.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #62      +/-   ##
==========================================
+ Coverage   95.85%   95.89%   +0.03%     
==========================================
  Files          31       31              
  Lines        5041     5037       -4     
==========================================
- Hits         4832     4830       -2     
+ Misses        209      207       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Co-authored-by: Ludovico Bianchi <lbianchi@lbl.gov>

where :math:`n_{t,prec,c}` is the outlet precipitacion of c component, :math:`n_{t,aq_in,c}` is the inlet of c comp in
the aqueous phase, :math:`n_{t,aq_in,c}` is the outlet of c comp in the aqueous phase at time :math:`t`, divided by the
stechiometric parameter of component c :math:'{S{comp}'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
stechiometric parameter of component c :math:'{S{comp}'
stechiometric parameter of component c :math:`S{comp}`

Copy link
Contributor

Choose a reason for hiding this comment

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

Also - stoichiometric is spelt incorrectly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've added it for consideration for the next update to our favorite spellchecker: crate-ci/typos#956 (comment) (not sure if or when it'll become active, though, so we'll need to check for it manually until then)

@lbianchi-lbl
Copy link
Contributor

@agarciadiego Python modules should be added to docs/api.rst to be included in the docs, e.g.

API Reference
=============

Leaching
--------

.. autosummary::
   :toctree: _autosummary

   prommis.leaching.leach_reactions
   prommis.leaching.leach_solids_properties
   prommis.leaching.leach_solution_properties

Roasting
--------

.. autosummary::
   :toctree: _autosummary

   prommis.roasting.ree_feed_roaster
   prommis.roasting.ree_oxalate_roaster

Precipitate
-----------

.. autosummary::
   :toctree: _autosummary
   
   prommis.precipitate.precipitator
   prommis.precipitate.precipitate_liquid_properties
   prommis.precipitate.precipitate_liquid_properties

@andrewlee94
Copy link
Contributor

@lbianchi-lbl Does that mean that the new LeachTrain model/class should be added here as well?

@agarciadiego agarciadiego marked this pull request as ready for review April 26, 2024 14:12
docs/api.rst Outdated Show resolved Hide resolved
@lbianchi-lbl
Copy link
Contributor

@lbianchi-lbl Does that mean that the new LeachTrain model/class should be added here as well?

I've just checked and yes, I think the module prommis.leaching.leach_train should be added there.

@agarciadiego
Copy link
Contributor Author

@lbianchi-lbl Does that mean that the new LeachTrain model/class should be added here as well?

I've just checked and yes, I think the module prommis.leaching.leach_train should be added there.

I'll add it in this PR

@andrewlee94
Copy link
Contributor

@agarciadiego Thank you.

agarciadiego and others added 3 commits April 26, 2024 11:01
Co-authored-by: MarcusHolly <96305519+MarcusHolly@users.noreply.github.com>
Co-authored-by: MarcusHolly <96305519+MarcusHolly@users.noreply.github.com>
Co-authored-by: MarcusHolly <96305519+MarcusHolly@users.noreply.github.com>
Co-authored-by: MarcusHolly <96305519+MarcusHolly@users.noreply.github.com>
@MarcusHolly
Copy link
Contributor

The formatting of the leach train documentation got a bit messed up as a result of the indents. Other than this, the PR LGTM.

image

@ksbeattie ksbeattie added Priority:High High Priority Issue or PR and removed Priority:Normal Normal Priority Issue or PR labels Apr 29, 2024
@@ -1204,22 +1194,13 @@
"WARNING (W1002): Setting Var 'fs.leach.liquid[0.0,2].conc_mol_comp[Dy]' to a\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these warnings be resolved, or hidden?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could skip the validation of the variables. However, I don't know if this would be a positive change. Maybe we should add an issue. What do you think @MarcusHolly

Copy link
Contributor

Choose a reason for hiding this comment

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

We can create an issue in the spirit of getting this PR merged today or tomorrow.

]
}
],
"outputs": [],
"source": [
"# This display needs to be replaced with m.fs.roaster.report() when available\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

There is an extra empty cell at the end of the notebook.

@agarciadiego agarciadiego enabled auto-merge (squash) April 30, 2024 16:53
@agarciadiego agarciadiego merged commit 88d17dc into prommis:main Apr 30, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request Priority:High High Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants