Parity with legacy MPRester convenience functions

This issue is to discuss what necessary parity should exist between the new and legacy MPRester top level convenience functions (e.g. MPRester.get_entries_in_chemsys).

Below is list of current breaking differences going from legacy to new.

Missing:

submit_vasp_directory
delete_snl
get_all_substrates
get_cohesive_energy
get_data
get_doc
get_download_info
get_exp_entries
get_exp_thermo_data
get_interface_reactions
get_phonon_ddb_by_material_id
get_reaction
get_stability
submit_vasp_directory
submit_snl
query_snl
parse_criteria

Major changes:

query replaced with MPRester.summary.search. No MongoDB-like query language support anymore.
get_gb_data replaced with MPRester.grain_boundary.search
get_substrates replaced with MPRester.substrates.search
get_surface_data replaced with MPRester.surface_properties.search
get_task_data replaced with MPRester.tasks.search
get_xas_data replaced with MPRester.xas.search
get_entries:
Removed args compatible_only, inc_structure, property_data, conventional_unit_cell.
get_entries_in_chemsys:
Removed args compatible_only, inc_structure, property_data, conventional_unit_cell, additional_criteria.
get_entry_by_material_id:
Removed args compatible_only, inc_structure, property_data, conventional_unit_cell.
submit_structures:
Replaced args authors, projects, references, remarks, data, histories, created_at with public_name, public_email.

@shyuep

@shyuep this is current as of v0.27.3

Based on the above, I think the only ones I think warrant feature parity are get_entries*. I know of course that inc_structure is irrelevant in the new API, but I think you should just leave the arg in, default it to None, and just have a warning message if someone puts in a value to say that this argument is maintained for backwards compatibility and has no effect in the new API. I think compatible_only, additional_criteria and property_data needs to be implemented for sure - those are often used. I am not sure where conventional_structure fits in here, since ComputedStructureEntries are already returned. I personally never used that, but maybe someone else did.

Rather than add deprecated kwargs to the docstring which do nothing, we could just add a catch-all **kwargs and warn if any additional ones are provided?

I think it is better to be explicit. E.g., if someone tries nonsense=True, we want the code to fail with "nonsense is not a supported kwarg" rather than a warning. There is only one arg that is redundant right now - inc_structure. The rest actually does something.

I'm fine with it raising instead of warning, it was more cluttering the docstring that I was asking about. But no strong feelings either way.

if kwargs:
    # or even check for the specific list of kwargs here
    raise ValueError("{','.join(kwargs.keys()} are not supported keyword arguments. These may only have been supported by the legacy API.")

Yeah, I prefer the documentation to say something about it too, since this is a clear change from previous behavior. Also, it is generally poor practice to allow an arbitrary list of args and then have to check it for validity. In any case, this is not a detail I want to bother debating over. So I will leave you guys to implement whichever way you want. I am more concerned with the broader issues of feature parity for the args that actually does somehting as well as broken documentation/features in the API.

@munrojm

Hi @munrojm, I want to follow up in this issue about the missing "get_cohesive_energy". Is there a way to get the cohesive energy or isolated energy of elements now? Previously, the MPRester could do that with _make_request, while now I'm wondering how to get the isolated elemental energies.

@JiQi535

@JiQi535, this is currently being worked on. The isolated atom calculations will be incorporated into the API very soon. After that I can write a method for the client.

@shyuep

@shyuep, the updates to the methods are in the client. The only two arguments that don't have their behavior at parity are additional_criteria and inc_structure. For both of those, the user is warned if either is passed and the docstrings also list them as deprecated. Here is the relevant PR: #691

Thanks. But additional_criteria shouldn't be deprecated. It is actually immensely useful.

@shyuep

@shyuep The issue is that the MongoDB-like query syntax you would pass to that argument is not supported anymore. The updated alternative would be to pass a dictionary mapping between the new query arguments of MPRester.thermo.search and their values for additional criteria. This seems strange to me, as it departs significantly from the legacy behavior (requiring users to change their code anyway), and points to just using the MPRester.thermo.search method directly.

Can't we just pass **additional_criteria as kwarg to the query to set the criteria? If this is undoable, at the min allowing such a query, even if the syntax has to change, is preferable to not allowing it at all.

Yes, but will just be like passing **additional_criteria directly to MPRester.thermo.search. If you feel strongly about it and think we should do it, then I will update and patch release.

I think it is non obvious that entries come from Thermo endpoint. Entries have many functions other than thermo. Anyway, since it does not detract from functionality and would help backwards compatibility, we should just add it.

Okay, I have updated the client and have done a patch release.

The get_all_substrates in legacy MPRester returns a list of material_ids ("/materials/all_substrate_ids") corresponding to possible substrates. Is such a list of material_ids also used by substrates.search?
For example, I have tried insertion_electrodes.search(fields=['battery_id']) and get all battery ids. However, a similar search on substrates returns error ("Server timed out trying to obtain data. Try again with a smaller request").
The list of substrates is desired when a local film structure without material_id is considered. Thank you.

@goodwilling

@goodwilling which exact query are you trying to make? I can take a look at this.

Thanks. The query that I made is as follows:

mpr= MPRester(API_key)
docs = mpr.substrates.search(fields=['sub_id'])

@esoteric-ephemera

get_cohesive_energy was implemented in #951 by @esoteric-ephemera today after additional requests through the forum. A release candidate for it will be published soon.

@goodwilling mpr.materials.substrates.search() should not be timing out anymore but will still take a while since there are 10M documents to download. Internally reorganizing the document structure for the substrates collection is on our TODO list.

Navigation Menu

Repository navigation

Parity with legacy MPRester convenience functions #676

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Parity with legacy MPRester convenience functions #676

Description

Activity

munrojm commented on Sep 22, 2022

shyuep commented on Sep 23, 2022

mkhorton commented on Oct 3, 2022

shyuep commented on Oct 3, 2022

mkhorton commented on Oct 3, 2022

shyuep commented on Oct 3, 2022

JiQi535 commented on Oct 10, 2022

munrojm commented on Oct 11, 2022

munrojm commented on Oct 11, 2022

shyuep commented on Oct 11, 2022

munrojm commented on Oct 11, 2022

shyuep commented on Oct 11, 2022

munrojm commented on Oct 12, 2022

shyuep commented on Oct 12, 2022

munrojm commented on Oct 13, 2022

goodwilling commented on Feb 8, 2023

munrojm commented on Feb 9, 2023

goodwilling commented on Feb 9, 2023

tschaume commented on Dec 14, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions