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 __call__ method to with_units #1650

Merged
merged 2 commits into from
Dec 24, 2023
Merged

Conversation

mefuller
Copy link
Contributor

@mefuller mefuller commented Dec 7, 2023

Changes proposed in this pull request

add __call__ to cantera.with_units solutions (closes #1649)

Checklist

  • The pull request includes a clear description of this code change
  • Commit messages have short titles and reference relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • Style & formatting of contributed code follows contributing guidelines
  • The pull request is ready for review

Copy link
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

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

Thanks @mefuller! Can you back out the formatting-only changes here? We intentionally don't follow black's formatting.

Also, I think the ThermoPhase.report() method should be copied over, and that should be used for printing. This looks like it's generating a SolutionArray, like maybe you copied the __call__ from that class?

@mefuller
Copy link
Contributor Author

@bryanwweber oops - forgot that was turned on at work - meant to get back to this anyway at home, so here is the corrected commit

Looks like this doesn't actually fix the problem and I'm not sure why (I did a scons build/test/install and I still get the same bug - any ideas as to what I did wrong (I definitely don't understand 100% what's going on here in any case)

@bryanwweber
Copy link
Member

@mefuller can you open up the generated solution.py in the build folder? That'll show you the actual code being run. I also edited my comment above with a suggestion for the implementation

@mefuller mefuller changed the title add __call__ method to with_units (via blind copy-pasta) add __call__ method to with_units Dec 11, 2023
@mefuller mefuller marked this pull request as ready for review December 12, 2023 02:35
@mefuller
Copy link
Contributor Author

@bryanwweber looks like the CI / .NET failures are due to a 404 pulling down dependencies - is there an easy way to re-trigger these?

@bryanwweber
Copy link
Member

@mefuller I just restarted them

@speth
Copy link
Member

speth commented Dec 14, 2023

I think the failure of the .NET on ubuntu-22.04 run is because we're missing a sudo apt update before the sudo apt install ... command in that workflow (though I don't really understand the circumstances under which older package versions are removed from the Ubuntu repositories).

The other errors with the .NET interface are already fixed in the main branch (see commit aee3092), and can be resolved by rebasing.

@mefuller
Copy link
Contributor Author

@bryanwweber the failures in CI seem to all be on .NET (with which I am not at all familiar) - any idea what might be the cause?

@bryanwweber
Copy link
Member

I'm not sure, I think some foxes are on main if you haven't rebased in a bit. Maybe @speth can help 😟

@speth
Copy link
Member

speth commented Dec 14, 2023

The solution to both distinct errors is in my previous comment.

@mefuller
Copy link
Contributor Author

Sorry7 about glossing over that @speth - rebased and rerunning

@bryanwweber
Copy link
Member

Thanks @mefuller , the code looks good, I just haven't had a chance to pull it down and try it out. I'll try to do that soon!

Copy link

codecov bot commented Dec 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (87317b7) 72.68% compared to head (414d363) 72.68%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1650      +/-   ##
==========================================
- Coverage   72.68%   72.68%   -0.01%     
==========================================
  Files         370      370              
  Lines       56302    56302              
  Branches    20403    20403              
==========================================
- Hits        40923    40922       -1     
- Misses      12371    12372       +1     
  Partials     3008     3008              

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

@bryanwweber bryanwweber merged commit de4e7c2 into Cantera:main Dec 24, 2023
48 of 49 checks passed
@bryanwweber
Copy link
Member

Thanks @mefuller!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to call solution object when using units
3 participants