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 function to save the solution #214

Merged
merged 4 commits into from
Nov 14, 2023
Merged

Add function to save the solution #214

merged 4 commits into from
Nov 14, 2023

Conversation

hwpang
Copy link
Contributor

@hwpang hwpang commented May 14, 2023

No description provided.

@codecov
Copy link

codecov bot commented May 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (eed1896) 49.82% compared to head (7074e9c) 49.91%.
Report is 21 commits behind head on main.

❗ Current head 7074e9c differs from pull request most recent head a165085. Consider uploading reports for the commit a165085 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #214      +/-   ##
==========================================
+ Coverage   49.82%   49.91%   +0.08%     
==========================================
  Files          31       31              
  Lines        8086     7920     -166     
==========================================
- Hits         4029     3953      -76     
+ Misses       4057     3967      -90     

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

@hwpang hwpang requested a review from xiaoruiDong May 18, 2023 19:21
Copy link
Collaborator

@mjohnson541 mjohnson541 left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me. Should we add a read function to go with the save function?

function save(syss::T, save_name::String) where {T<:SystemSimulation}
df = DataFrame(syss.sol)
for sim in syss.sims
rename!(df, names(df)[sim.domain.indexes[1]:sim.domain.indexes[2]] .=> sim.names)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible for us to have a name collision if for example you have water in gas phase and liquid phase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I postfixed the phase name to the species names in the new push.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you squash the fix in here?

@hwpang
Copy link
Contributor Author

hwpang commented Oct 31, 2023

I rebased with main. @mjohnson541 can you review & merge after the tests pass?

Copy link
Collaborator

@mjohnson541 mjohnson541 left a comment

Choose a reason for hiding this comment

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

A couple comments.

@@ -4,36 +4,36 @@ using Sundials

@testset "Test Reactors" begin

phaseDict = readinput("../src/testing/liquid_phase.rms")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is going to create nasty merge conflicts with other PRs can we hold off on this until right before v1.0.0?

function save(syss::T, save_name::String) where {T<:SystemSimulation}
df = DataFrame(syss.sol)
for sim in syss.sims
rename!(df, names(df)[sim.domain.indexes[1]:sim.domain.indexes[2]] .=> sim.names)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you squash the fix in here?

@hwpang
Copy link
Contributor Author

hwpang commented Nov 14, 2023

Looks mostly good to me. Should we add a read function to go with the save function?

I don't think it makes sense to add it into this PR, but I agree it would be nice to have a load function. We can open up an issue for that.

@hwpang
Copy link
Contributor Author

hwpang commented Nov 14, 2023

@mjohnson541 I addressed the comments!

@@ -608,15 +610,79 @@ end;

inter,pinter = FragmentBasedReactiveFilmGrowthInterfaceConstantT(domainfilm,domainliq,interfacerxns);

react,y0,p = Reactor((domainfilm,domainliq),(y0film,y0liq),(0.0,0.1),(inter,),(pfilm,pliq,pinter));
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are all these other changes here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops probably something went wrong when I rebased this. Will fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@mjohnson541 mjohnson541 merged commit 92a8b09 into main Nov 14, 2023
1 of 2 checks passed
@mjohnson541 mjohnson541 deleted the save_sol branch November 14, 2023 03:01
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.

2 participants