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 basis to conc example on property naming conventions docs #1482

Merged

Conversation

kurbansitterley
Copy link
Contributor

@kurbansitterley kurbansitterley commented Sep 4, 2024

Fixes #1481

Add basis to conc example on property naming conventions docs page.
see #1481

Summary/Motivation:

Concentration needs a basis (mass, mol, etc) to be a valid property name but was not included in the docs example.

Changes proposed in this PR:

  • Add basis to conc example on property naming conventions docs page.

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.

@dangunter
Copy link
Member

dangunter commented Sep 4, 2024

spelling issue is due to an update in typos.

@lbianchi-lbl
Copy link
Contributor

@lbianchi-lbl Do you know why the spell checker would fail on a DMF file, which certainly wasn't altered by this PR?

The first reason that comes to mind is that a new version of the spellchecker was released, with new typos being added, which would cause typos that used to be ignored with previous versions to be flagged instead.

This is a consequence of the typos package version being unpinned. This was a conscious decision we took being aware that it would result in occasional failures in unrelated PRs, but I've been thinking about reconsidering this and make the version update (and therefore the possibility of new typos being flagged) be "opt-in" instead.

TL;DR New version of spellchecker flags new typos that were already present in the codebase.

@bpaul4
Copy link
Contributor

bpaul4 commented Sep 4, 2024

@lbianchi-lbl @dangunter we could pin between releases, and on each release cycle update the version and fix any flagged typos. Might be a "best of both worlds" approach so PRs don't fail when typos pushes a new version.

@@ -223,7 +223,7 @@ Variable Name Meaning
enth Specific enthalpy of the entire mixture (across all phases)
flow_comp["H2O"] Total flow of H2O (across all phases)
entr_phase["liq"] Specific entropy of the liquid phase mixture
conc_phase_comp["liq", "H2O"] Concentration of H2O in the liquid phase
conc_mass_phase_comp["liq", "H2O"] Concentration of H2O in the liquid phase on a mass basis
Copy link
Member

Choose a reason for hiding this comment

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

As you noted in the associated issue, there are a few others here that need to be updated with a baisis. Would you be able to update the following

enth to enth_mol
flow to flow_vol
entr_phase['Liq'] to entr_mol_phase['Liq']

(I chose a variety of bases to demonstrate their uses).

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.

Thank you for noting this - this is important for understanding the naming convention and was definitively wrong (and thus misleading).

@andrewlee94 andrewlee94 added bug Something isn't working documentation Documentations comments and requests Priority:High High Priority Issue or PR labels Sep 4, 2024
@lbianchi-lbl lbianchi-lbl enabled auto-merge (squash) September 5, 2024 20:51
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.36%. Comparing base (8140a3d) to head (8b93059).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1482      +/-   ##
==========================================
- Coverage   76.37%   76.36%   -0.01%     
==========================================
  Files         393      393              
  Lines       65086    65086              
  Branches    14426    14426              
==========================================
- Hits        49708    49705       -3     
- Misses      12816    12820       +4     
+ Partials     2562     2561       -1     

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

@lbianchi-lbl lbianchi-lbl merged commit 8ebf0b8 into IDAES:main Sep 6, 2024
8 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Documentations comments and requests Priority:High High Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Property naming conventions for vapor pressure and saturation vapor pressure
6 participants