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

Update looptool documentation #1524

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

johnson12742
Copy link

Fixes/Resolves:

Update the loop_tool documentation

Summary/Motivation:

Looptool location has not yet been updated in the docs. Also, requirement for looptool inputs have not been mentioned. This PR updates the documentation to address these issues.

Changes proposed in this PR:

  • Direct users to the new location of loop_tool
  • Let users know that loop_tool inputs must be literals

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.

@johnson12742
Copy link
Author

@lbianchi-lbl One of the checks is failing (though it is not a required). I re-ran it (in case it was a one off) but that didn't solve anything.
I only changed two lines in the looptool documentation.

@lbianchi-lbl
Copy link
Contributor

This looks like a server-side error on the Codecov side, so unfortunately it's unlikely we can do anything to fix it if it's not addressed by them. I couldn't find any related issue so I opened one: codecov/feedback#577

@lbianchi-lbl
Copy link
Contributor

A couple of thoughts:

  • There are more occurrences of the outdated import path: https://github.com/search?q=repo%3Awatertap-org%2Fwatertap+watertap.tools.analysis_tools.loop_tool&type=code
  • The reason why this wasn't caught earlier is that the code snippets in the Loop tool guide use .. code-block instead of .. testcode directives, causing them to be ignored by the doctests: compare e.g.
    An example of the functions setup for use with loopTool for RO_with_energy_recovery flowsheet example.
    .. code-block::
    import watertap.flowsheets.RO_with_energy_recovery.RO_with_energy_recovery as ro_erd
    def ro_build(erd_type=None, **kwargs):
    m = ro_erd.build(erd_type)
    return m
    def ro_init(m, solver=None, **kwargs):
    ro_erd.set_operating_conditions(
    m, water_recovery=0.5, over_pressure=0.3, solver=solver
    )
    ro_erd.initialize_system(m, solver=solver)
    ro_erd.optimize_set_up(m)
    def ro_solve(m, solver=None, **kwargs):
    result = solver.solve(m)
    return result

    As before, we begin by importing or explicitly programming any functions relating to flowsheet building/specification, simulation, and optimization setup steps. We will use the same RO with energy recovery flowsheet for this example.
    .. testsetup::
    # quiet idaes logs
    import idaes.logger as idaeslogger
    idaeslogger.getLogger('ideas.core').setLevel('CRITICAL')
    idaeslogger.getLogger('ideas.core.util.scaling').setLevel('CRITICAL')
    idaeslogger.getLogger('idaes.init').setLevel('CRITICAL')
    .. testcode::
    # replace this with your own flowsheet module, e.g.
    # import my_flowsheet_module as mfm
    import watertap.flowsheets.RO_with_energy_recovery.RO_with_energy_recovery as RO_flowsheet
    Once this is done, we import the differential parameter sweep tool and sampling classes.
    .. testcode::
    from parameter_sweep import differential_parameter_sweep, UniformSample, NormalSample
  • Ideally, we should replace the .. code-block directives with .. testcode so that these code blocks are tested as part of our CI checks, reducing the chances for future issues with outdated code to go unnoticed

@johnson12742
Copy link
Author

@lbianchi-lbl So, what would you suggest? I am happy to close this PR if you think there is a better way to update the docs (compared to what I am doing).

@lbianchi-lbl
Copy link
Contributor

@lbianchi-lbl So, what would you suggest? I am happy to close this PR if you think there is a better way to update the docs (compared to what I am doing).

@johnson12742 sorry for the confusion. To clarify, I think the changes in this PR are good (and, BTW, thanks for making them). The items I listed in my previous message are things that I think we should consider doing in addition to the changes you've made.

Copy link
Contributor

@adam-a-a adam-a-a left a comment

Choose a reason for hiding this comment

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

Sorry- approved and then saw @lbianchi-lbl 's comments. If we want to address those in this PR, I will take my review back

@johnson12742
Copy link
Author

@lbianchi-lbl I didn't see any other instances where the documentation needs to be updated. I think we are good to go.

@lbianchi-lbl
Copy link
Contributor

lbianchi-lbl commented Nov 21, 2024

@johnson12742 these are the lines that (as of d80a993) still have a reference to the invalid import path:

https://github.com/johnson12742/watertap/blob/d80a99328ebb9cec8f45a44d769380204e7ae376/docs/how_to_guides/how_to_use_loopTool_to_explore_flowsheets.rst?plain=1#L1-L8

@johnson12742
Copy link
Author

@lbianchi-lbl Thanks for catching that. I don't know why it didn't show up when I searched

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:Normal Normal Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants