Parity with legacy MPRester convenience functions

[munrojm]

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.

[munrojm]

@shyuep this is current as of v0.27.3

[shyuep]

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.

[mkhorton]

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?

[shyuep]

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.

[mkhorton]

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.") 

[shyuep]

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.

[JiQi535]

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.

[munrojm]

@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.

[munrojm]

@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: https://github.com/materialsproject/api/pull/691

[shyuep]

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

[munrojm]

@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.

[shyuep]

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.

[munrojm]

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.

[shyuep]

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.

[munrojm]

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

[goodwilling]

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.

[munrojm]

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

[goodwilling]

Thanks. The query that I made is as follows:

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