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

Fixed the parity plot function legend settings #669

Merged
merged 1 commit into from
Jun 29, 2023
Merged

Fixed the parity plot function legend settings #669

merged 1 commit into from
Jun 29, 2023

Conversation

MichalKesl
Copy link
Contributor

Fixed the legend settings for the parity plot and changed the units and the title of the plot to be written in Latex.

@codecov
Copy link

codecov bot commented Jun 5, 2023

Codecov Report

Merging #669 (2fa44ba) into main (677169a) will decrease coverage by 0.32%.
The diff coverage is 0.00%.

❗ Current head 2fa44ba differs from pull request most recent head 5c19c89. Consider uploading reports for the commit 5c19c89 to get more accurate results

@@            Coverage Diff             @@
##             main     #669      +/-   ##
==========================================
- Coverage   73.48%   73.17%   -0.32%     
==========================================
  Files          99       99              
  Lines       26488    26427      -61     
  Branches     5541     5535       -6     
==========================================
- Hits        19466    19338     -128     
- Misses       5659     5722      +63     
- Partials     1363     1367       +4     
Flag Coverage Δ
unittests 73.17% <0.00%> (-0.32%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
arc/plotter.py 27.64% <0.00%> (-0.13%) ⬇️

... and 9 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kfir4444
Copy link
Collaborator

kfir4444 commented Jun 5, 2023

Thanks for the contribution @MichalKesl! Great job!
Maybe we can use this opportunity to talk about how to test this function?

Copy link
Member

@alongd alongd left a comment

Choose a reason for hiding this comment

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

Thanks @MichalKesl! I added some minor comments and questions. Let's test it in an ARC run for thermo

arc/plotter.py Outdated Show resolved Hide resolved
arc/plotter.py Outdated Show resolved Hide resolved
arc/plotter.py Show resolved Hide resolved
arc/plotter.py Outdated Show resolved Hide resolved
@kfir4444
Copy link
Collaborator

kfir4444 commented Jun 6, 2023

thermo_parity_plots.pdf
Here is an example of the parity plot using this branch.
One important comment on that is the parenthesis should be using \left(, \right), to make sure that they match the size of the \frac that is inside them.
Also, I would have used \cdot to represent multiplication in the denominator of the entropy units, to differentiate the mol and K

@alongd
Copy link
Member

alongd commented Jun 6, 2023

Thanks! While we make the titles fancier, could you make H298 and S298 be something like \Delta H ^{298_K} _f}?

@MichalKesl MichalKesl force-pushed the squared branch 2 times, most recently from 183199a to 54106f1 Compare June 7, 2023 07:35
Copy link
Member

@alongd alongd left a comment

Choose a reason for hiding this comment

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

Thanks! Can you take a look at one pending comment?
Can you upload an example of the plot?

arc/plotter.py Show resolved Hide resolved
@alongd
Copy link
Member

alongd commented Jun 11, 2023

@MichalKesl, thanks, looking good.
Could you please clean the commit message/description and rebase on main?

Copy link
Collaborator

@kfir4444 kfir4444 left a comment

Choose a reason for hiding this comment

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

Great job! LGTM.

@kfir4444 kfir4444 merged commit af4a9d6 into main Jun 29, 2023
@kfir4444 kfir4444 deleted the squared branch June 29, 2023 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants