Remove `is_rare_earth_metal`

[jevandezande]

Python version

All

Pymatgen version

Master

Current behavior

Throws deprecation warning in CI

Expected Behavior

Removal

Deadline to remove 2025-01-01

pymatgen/src/pymatgen/core/periodic_table.py

Lines 757 to 768 in 1abc459

	@property
	@deprecated(
	is_rare_earth,
	message="is_rare_earth is corrected to include Y and Sc.",
	deadline=(2025, 1, 1),
	)
	def is_rare_earth_metal(self) -> bool:
	"""True if element is a rare earth metal, Lanthanides (La) series and Actinides (Ac) series.
	This property is Deprecated, and scheduled for removal after 2025-01-01.
	"""
	return self.is_lanthanoid or self.is_actinoid

[naik-aakash]

CI failing on downstream packages

JaGeo/LobsterPy#357

[DanielYang59]

Happy new year and sorry guys for the first bug of 2025, looks like the repo checking mechanism in monty.dev.deprecated is not doing what I expected it to do

It should only raise the warning as error in the code owner's repo (in this case it should only error out in pymatgen's CI not forks) but apparently it's also triggered elsewhere, would fix this from monty's side ASAP

[ml-evs]

Came here to report this, surely it should not "raise" a DeprecationWarning at all, but warn? This way the pymatgen CI can still error on it, but it allows downstream providers to configure it to be ignored. Having something time limited like this is also really unpleasant for dependencies -- especially since it triggers on import rather than use -- as using older versions of this package that may have been operating completely fine will now error out.

[DanielYang59]

Hi @ml-evs happy new year, thanks for the comment :)

surely it should not "raise" a DeprecationWarning at all, but warn? This way the pymatgen CI can still error on it, but it allows downstream providers to configure it to be ignored.

I believe this was intended to be raised such that it would error out in the code owner's repo as a reminder for action (though apparently it's not doing what we wanted).

However I just realized in materialyzeai/monty#735 that this may not be (easily) possible as a downstream package could use a deprecated upstream function/class and still get this warning/error in its upstream CI (unless we check the call stack depth but this sounds a bit fragile).

Also even as a warning, downstream would have to filter out such warning, it still could be annoying because:

• This warning is not helpful/actionable for downstream package (they cannot remove a deprecated upstream function/class)
• I wish to limit this to the code owner and minimize actions needed by downstream packages (including adding such filters), it was intended as a reminder for the specific package

But I cannot think of a way to resolve this at this moment. So perhaps let's drop this deadline warning/error altogether for now (still keep the deadline in the decorator and msg as a reminder) until someone has a way around this?

[ml-evs]

Agree this needs to be fixed at the monty level (and ideally urgently), as right now it renders every old pymatgen version unusable. I don't understand the rationale for raising, most packages should run their tests with warnings as errors and maintain a list of allowed warnings for deprecations (this should be a conscious choice). I guess pymatgen doesn't currently do this in its CI given the mulititude of warnings that arise; instead you could add something like

[tool.pytest.ini_options]
filterwarnings = [
   "ignore", # essentially the current behaviour
   "error:.*This function should have been removed.*:DeprecationWarning"
]

to mimic the current behaviour without breaking downstream packages.

I understand the motivation behind the deadline but it really should be a choice whether it is obeyed -- I would also much prefer monty to not poke around with my git settings or make system calls when checking this. I wouldn't be against it warning in every scenario though, though preferably this decorator should only trigger when the function is called, rather than imported.

[DanielYang59]

I don't understand the rationale for raising, most packages should run their tests with warnings as errors and maintain a list of allowed warnings for deprecations (this should be a conscious choice).

I agree that was a bad decision, warn would be much gentle

I guess pymatgen doesn't currently do this in its CI given the mulititude of warnings that arise; instead you could add something like

Thanks for the input, pymatgen does have this filter but we could/should have been more careful about that monty decision in the first place, apologize again:

pymatgen/pyproject.toml

Lines 246 to 257 in 1abc459

	[tool.pytest.ini_options]
	addopts = "--durations=30 --quiet -r xXs --color=yes --import-mode=importlib"
	filterwarnings = [
	# NOTE: the last matching option is used
	"ignore::Warning", # Ignore all Warning
	"default::FutureWarning", # Show FutureWarnings
	"default::DeprecationWarning", # Show DeprecationWarnings
	# TODO: pybtex (perhaps some others) emits the following warnings
	'ignore:pkg_resources is deprecated as an API:DeprecationWarning',
	'ignore:distutils Version classes are deprecated:DeprecationWarning',
	'ignore:Deprecated call to `pkg_resources.declare_namespace:DeprecationWarning',
	]

I understand the motivation behind the deadline but it really should be a choice whether it is obeyed -- I would also much prefer monty to not poke around with my git settings or make system calls when checking this. I wouldn't be against it warning in every scenario though

I believe such config checking was intended to minimize impact to repos other than the code owner's (apparently I failed to achieve this goal) because other than that we didn't come up with a solution to decide the code owner.

Perhaps let's drop this warning altogether for now (as a hot fix) and implement better solutions later if there's any. I personally would prefer not be broadcast such a warning at all if I cannot find a way to confine such warning to its desired audience (and do a manual check of "overdue" deprecation from time to time maybe).

[ml-evs]

Perhaps let's drop this warning altogether for now (as a hot fix) and implement better solutions later if there's any. I personally would prefer not be broadcast such a warning at all if I cannot find a way to confine such warning to its desired audience (and do a manual check of "overdue" deprecation from time to time maybe).

That's fair enough, its a tricky balance and I appreciate the aim of all of this was a noble one!