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

Flowsheet Documentation for GAC #1396

Merged
merged 21 commits into from
May 31, 2024

Conversation

hunterbarber
Copy link
Contributor

Fixes/Resolves:

GAC flowsheet documentation for #1219

Summary/Motivation:

Adding GAC flowsheet documentation

Changes proposed in this PR:

  • Only includes adding .rst for the GAC flowsheet

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.

@hunterbarber hunterbarber added Priority:High High Priority Issue or PR 1.0 Hard requirement for the 1.0 release labels May 20, 2024
@hunterbarber hunterbarber self-assigned this May 20, 2024
Implementation
--------------

As a single unit operation, the assumptions for the flowsheet are aligned with those detailed in the :doc:`GAC unit model documentation <../unit_models/gac>`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MarcusHolly I'm going to point you to this because I see you were hard linking some internal documentation hyperlinks in the flowsheet documentation. I think this method is better because it should link to the version of the documentation the reader is using instead of having the hard link to latest. But honestly someone more familiar than me with .rst compiling should give a recommendation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the suggestion Hunter. I definitely think this is better than hard-coding. This change should probably be applied across the board tho (not just flowsheet documentation) since I believe most links are hard-coded. This probably warrants it's own PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I was looking through the docs to see how it is done to get this one in, and the answer is that it depends. I think all the core files like getting started and how-to's use .rst utilities instead of hard linking, but they rely on establishing a name at the top of certain documentation files (which the unit models don't have but the how-to's and others do) and then referencing the name inline, instead of here which references the file path tree to the desired link.

Copy link
Contributor

Choose a reason for hiding this comment

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

From a quick skim, it looks like some flowsheets, unit models, and one property model documentation are hard-coding links

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the best solution it to just change the hard-coded link from latest to stable because a user might have an outdated version of the documentation, and we would still want the links to re-direct them to more up-to-date documentation. I see that some documentation does this already (getting_started.rst, how_to_setup_simple_chemistrt.rst, etc.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO you would always want the version to stay the same before and after clicking the link which you can't get through hard-coding, but yea this would be a tedious review of the current documentation.

Copy link

codecov bot commented May 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.01%. Comparing base (77d096e) to head (baf5e8f).
Report is 53 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1396   +/-   ##
=======================================
  Coverage   94.01%   94.01%           
=======================================
  Files         335      335           
  Lines       35561    35561           
=======================================
  Hits        33431    33431           
  Misses       2130     2130           

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

Copy link
Contributor

@MarcusHolly MarcusHolly left a comment

Choose a reason for hiding this comment

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

One more comment

docs/technical_reference/flowsheets/gac.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@MarcusHolly MarcusHolly left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -96,6 +96,10 @@
#
html_favicon = "_static/favicon.ico"

# intersphinx mapping to idaes
intersphinx_mapping = {
Copy link
Contributor Author

@hunterbarber hunterbarber May 23, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@hunterbarber hunterbarber May 23, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

From the looks of it, intersphinx mapping seems like the better way to go--even though we have other ways of dealing with these links without hard-coding to a version from my vague recollection. Can't speak to the downsides though. @lbianchi-lbl @dangunter - any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the late reply, I missed the notification(s). I think InterSphinx mappings is a good approach. I've tested that the GAC links to the IDAES docs work as expected on this PR's ReadTheDocs build.

Copy link
Contributor

@savannahsakhai savannahsakhai left a comment

Choose a reason for hiding this comment

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

LGTM

@lbianchi-lbl lbianchi-lbl enabled auto-merge (squash) May 31, 2024 00:54
@lbianchi-lbl lbianchi-lbl merged commit ef4e4ef into watertap-org:main May 31, 2024
24 checks passed
@hunterbarber hunterbarber deleted the gac_flowsheet_doc branch May 31, 2024 13:12
k1nshuk pushed a commit to k1nshuk/watertap that referenced this pull request Jun 7, 2024
* writing doc for gac fs

* trying to reference unit model through hyperlink

* fixing indentation

* revising based on the readthedocs build

* revising based on the readthedocs build

* fixing indentation

* fixing indentation

* testing implementation of idaes linking

* fixing linking

* testing the different filepath formats

* testing the different filepath formats

* fixing the linking

* fixing the idaes linking

* changing punctuation

* changing to conventions

---------

Co-authored-by: Ludovico Bianchi <lbianchi@lbl.gov>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0 Hard requirement for the 1.0 release Priority:High High Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants