Support low-level query parameters in `search()`?

[computron]

See first comment in this issue.

[computron]

To add more comments to this:

The standard Materials Project API docs seems to suggest that nelements is a searchable field for the mp-api: https://api.materialsproject.org/docs#/Materials%20Summary/search_materials_summary_stats__get

However, the user reports the following code to not work:

from mp_api.client import MPRester
mpr = MPRester()
mpr.materials.summary.search(nelements=1)

I believe that the mp-api requires the nelements parameter to be set as num_elements, hence the error.

I think the user expectation is that the fields on the standard MP API docs web site can be directly searchable without having to remap them to a different nomenclature within the mp-api code. Perhaps any fields that are not recognized can map to their equivalent _search() method, but other solutions are also OK.

Note that the search routes do not fully match the MP API standard docs, e.g., nelements vs num_elements.

Edit: adding link to mp-api docs for reference: https://materialsproject.github.io/api/_autosummary/mp_api.client.routes.materials.summary.SummaryRester.html#mp_api.client.routes.materials.summary.SummaryRester.search

[tschaume]

Thanks @computron! As of now, the high-level search() function provides a subset of popular parameters for which it enables convenient search on numeric ranges with a single argument, i.e. num_elements=(1,3). Your second link contains the relevant documentation. The lower-level _search() function explained in our advanced usage section in the docs directly uses the list of query paramaters in your first link. Using mpr.materials.summary.search(num_elements=(1,3)) would be equivalent to mpr.materials.summary._search(nelements_min=1, nelements_max=3). mpr.materials.summary.search(num_elements=(1,1)) would return the same as mpr.materials.summary._search(nelements=1).

In first order, this seems to be an issue with the clarity of our documentation. While it's all there, users might get stuck in loops between the website's API page and the docs. I removed any links to the former in our docs and updated text to be clearer. I've also reduced text on the website's API page to avoid duplicated or outdated info which will go out with one of our next deployments. In general, we're unfortunately too strained with resources to keep our docs in perfect shape and rely on the community to help us by reaching out on the forum or suggesting edits through the "Edit on Github" button.

The suggested solution to support fallback on the lower-level query parameters in the standard search() function might be a way forward but requires some implementation cycles on our end that I can't provide a timeline for right now. We'd have to make sure to catch or transparently handle cases where both the current top-level num_elements and the low-level nelements are used and we'd have to ensure that the implemented solution also works for all sub-resters. There might also have been other reasons to keep search() and _search() separate when this was originally implemented. I closed PR #978 to avoid deprecating num_elements in search() and in favor of the potentially more resilient fallback solution.

[shyuep]

I don't think the solution that @esoteric-ephemera implemented should be closed. The implication seems to be that if there are no "cycles" to solve a problem completely, we will not going to even accept partial solutions that nonetheless move the needle in the right direction. Software implementation is ultimately about iterating towards a solution. The bug fix implemented by @esoteric-ephemera preserves backward compatibility, while partially addressing this issue. There were tests implemented that ensured that all existing functionality work as it should. It should be merged.

[tschaume]

Thanks for following up @shyuep! @esoteric-ephemera and I have discussed an approach that could help move things forward. This PR will be updated by early next week.

I'd like to better understand your argument re: backward compatibility. Could you send a code snippet that demonstrates the functionality which was previously supported in mp-api and now isn't? Thanks!

[shyuep]

@tschaume I was saying that there is backwards compatibility. There is nothing else needed in the PR.

[tschaume]

Yes, understood. I was asking for an example that demonstrates the backward compatibility issue which you think this PR addresses.

[tschaume]

Closed via #978