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

[WIP] bump monty #1095

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

naik-aakash
Copy link
Contributor

Changes

Upgrade monty to latest version

Reason

See materialsproject/pymatgen#4243, due to accidental oversight with deprecation warning raise behaviour in monty, all downstream packages where an earlier version of monty is used, CI tests fails

Copy link

codecov bot commented Jan 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 4.13%. Comparing base (4244da9) to head (6bf394e).
Report is 15 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #1095       +/-   ##
==========================================
- Coverage   72.82%   4.13%   -68.69%     
==========================================
  Files         187     187               
  Lines       13637   13627       -10     
  Branches     1370    1372        +2     
==========================================
- Hits         9931     564     -9367     
- Misses       3161   13032     +9871     
+ Partials      545      31      -514     

see 167 files with indirect coverage changes

@JaGeo
Copy link
Member

JaGeo commented Jan 4, 2025

Thanks @naik-aakash . But there seems to be another issue now 😬

@naik-aakash
Copy link
Contributor Author

Thanks @naik-aakash . But there seems to be another issue now 😬

Yeah Abinit parsers need to be updated in pymatgen seems 😅

@naik-aakash
Copy link
Contributor Author

Thanks @naik-aakash . But there seems to be another issue now 😬

Yeah Abinit parsers need to be updated in pymatgen seems 😅

Correction : Just a new release of pymatgen should be enough. Parsers in pymatgen are already updated

@DanielYang59
Copy link

Correction : Just a new release of pymatgen should be enough. Parsers in pymatgen are already updated

Yep I believe this has been fixed by materialsproject/pymatgen#4223

@naik-aakash
Copy link
Contributor Author

Hi @davidwaroquiers , @VicTrqt, @gpetretto : Can you please help me fixing the failing abinit tests on the upgrade of Monty and Pymatgen here ?

@VicTrqt
Copy link
Contributor

VicTrqt commented Jan 21, 2025

Hi @davidwaroquiers , @VicTrqt, @gpetretto : Can you please help me fixing the failing abinit tests on the upgrade of Monty and Pymatgen here ?

Hello @naik-aakash,
The failing abinit tests were due to slight mismatches in input variables, probably caused by recent updates in scipy codata values (7255094). To avoid regenerating the test data, we decided to allow a tolerance in the variables check. This required an update in abipy (dc6d097) and in atomate2. You can find these fixes in my branch with the open PR here: 784709d and 3fc9f2e. Since they are small, I thought you could integrate them into your current PR. Otherwise I can open a new one for the main instead of my branch under review.

@jmmshn
Copy link
Contributor

jmmshn commented Jan 21, 2025

Possible fix to the failing abinit tests:
#1106

Still validating. Looks like the problem is that:

  1. abipy is comparing floats
  2. the floats have be converted to string in this particular instance.

I think while we can wait for bump to the upstream abipy code and just hack in a manual skip of the one check for now.

The mismatch looks like this:

["The variable 'tsmear' is different in the two files:\n - this file: '0.0036749322175665 Ha'\n - other file: '0.0036749322175655 Ha'"]

But it sometimes shows up with the "Ha" and sometimes without (as if to mock us).

@jmmshn
Copy link
Contributor

jmmshn commented Jan 22, 2025

OK the abinit and aims tests that were breaking should be fixed by the PR above now.

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.

5 participants