Remove maggma dependence, add conversion electrode rester

[esoteric-ephemera]

Can be merged once emmet-core #1267 is merged/released.

• Migrates sole maggma import to emmet.core, removes maggma from dependencies.
• Adds ConversionElectrodeRester to query the conversion_electrodes endpoint

[esoteric-ephemera]

@tschaume there seem to be some issues with the conversion_electrodes endpoint, perhaps that's why a client search feature wasn't added for it before? Have basically flagged these in the class itself under _exclude_search_fields

• material_ids is listed as a valid search term in the docs, but there are neither material_id or material_ids fields in the schema
• formula, chemsys, and elements do not return any matches despite manually checking values from OpenData
• stability_charge and stability_discharge are also not working
• Similarly, passing exclude_elements will return the entire collection

[tschaume]

Looks good!

The issues you found are likely due to the the query operators defined here. The multi-material query isn't needed, for instance. It could be that it was a simple copy-paste oversight when the endpoint was added in emmet PR 814.

@kbuma maybe you could take a closer look at the operator definitions for the conversion electrodes endpoints as part of your cleanup in emmet PR 1282? Thanks!

[tschaume]

remove

[kbuma]

@kbuma maybe you could take a closer look at the operator definitions for the conversion electrodes endpoints as part of your cleanup in emmet PR 1282? Thanks!

@tschaume do you want me remove the ElectrodeMultiMaterialIDQuery() from the list of query operators for conversion_electrodes and insertion_electrodes endpoints? Or just the conversion one?

[tschaume]

@kbuma Yes, the multi-ID query can be removed for both endpoints. We should also double-check the other query parameters @esoteric-ephemera mentioned in his comment.

[kbuma]

@tschaume there seem to be some issues with the conversion_electrodes endpoint, perhaps that's why a client search feature wasn't added for it before? Have basically flagged these in the class itself under _exclude_search_fields

• material_ids is listed as a valid search term in the docs, but there are neither material_id or material_ids fields in the schema
• formula, chemsys, and elements do not return any matches despite manually checking values from OpenData
• stability_charge and stability_discharge are also not working
• Similarly, passing exclude_elements will return the entire collection

@tschaume here's my findings on this - please advise how you'd like to proceed:

• material_ids we already took care of and have removed from both electrode endpoint so @esoteric-ephemera can remove that from _exclude_search_fields
• formula, chemsys and elements resolve to checking against fields in the entries_composition_summary object in insertion_electrodes This field does not exist in conversion_electrodes I can remove the query operators for those from the router for conversion_electrodes on the server-side if you like; any other changes would require discussion on how to implement
• stability_charge and stability_discharge resolve to those fields in insertion_electrodes. They do not exist in conversion_electrodes I can remove the InsertionVoltageStepQuery from the router for conversion_electrodes on the server-side if you like; not sure if this makes sense to implement

[tschaume]

@kbuma Thanks for tracking this down! I'm getting the impression that the query operators were added in anticipation of the according fields being available in the endpoints but that never materialized. @esoteric-ephemera @tsmathis Do you think that adding these fields makes sense and is expected for the conversion/insertion_electrodes endpoints?

[esoteric-ephemera]

Those fields don't make as much sense for a conversion electrode - the electrode changes chemistry under dis-/charge so that makes any single formula ambiguous

Also worth noting that there's a get_summary_dict() method on the electrode_object in both the insertion and conversion electrodes collections, but most of the summary fields in conversion electrodes are already promoted to top-levels

[tschaume]

Thanks @esoteric-ephemera!

@kbuma Let's remove the query operators without according fields in the conversion_electrodes router. It seems that we can also get rid of get_summary_dict() in both.

[kbuma]

@tschaume I'll put together a PR for removing those and unused query operators for electrodes.

@esoteric-ephemera in develop branch they don't have a get_summary_dict() defined - it is referenced in a docstring but does not exist in electrode.py

[tsmathis]

haven't been following closely, but might be helpful for reference:

from what I've gathered the get_summary_dict method Aaron mentioned is kind of an as_dict alternative that only exists on the pymatgen's *Electrode classes: https://github.com/materialsproject/pymatgen/blob/ab34799d8ab5dee80756489cf2ca28a97de78121/src/pymatgen/apps/battery/insertion_battery.py#L307

so that method is available on the emmet.core.electrode.InsertionElectrodeDoc.electrode_object and emmet.core.electrode.ConversionElectrodeDoc.electrode_object fields.

that does get called during the builders to populate all of the electrode pair entries in the *ElectrodeDoc.adj_pairs fields: https://github.com/materialsproject/emmet/blob/c6921f6ebb0a07bb2841e76aba39de672b1a4c46/emmet-core/emmet/core/electrode.py#L84

[tsmathis]

@kbuma Thanks for tracking this down! I'm getting the impression that the query operators were added in anticipation of the according fields being available in the endpoints but that never materialized. @esoteric-ephemera @tsmathis Do you think that adding these fields makes sense and is expected for the conversion/insertion_electrodes endpoints?

yeah so the electrode docs don't inherit from emmet's base meta models edit: builder_meta is there, but not helpful for this (the one's that have all the typical material info), hence the lack of response from some of those. chemsys and elements should technically be a good options since they are top levels...

[tsmathis]

here are the top levels that are common to both insertion and coversion electrodes that could be useful:

    battery_type: BatteryType | None = Field(...   # this is always the same with each collection, so nvm

    battery_id: str | None = Field(...

    thermo_type: str | None = Field(...

    battery_formula: str | None = Field(...

    working_ion: ElementType | None = Field(...
    
    ...snip...

    framework: CompositionType | None = Field(...

    framework_formula: str | None = Field(...

    elements: list[ElementType] | None = Field(...

    nelements: int | None = Field(...
    
    chemsys: str | None = Field(...
    
    ...snip...

[tsmathis]

here are the top levels that are common to both insertion and coversion electrodes that could be useful:

although thermo_type is actually only used in the ConversionElectrodeDoc (all null in the insertion_electrodes collection) and probably should not have been added to the BaseElectrode model...

[esoteric-ephemera]

@kbuma these are in the pymatgen objects / electrode_object fields, ex. here

Whoops sorry I didn't see @tsmathis already answered

[esoteric-ephemera]

@tschaume I migrated all the various json.loads + MontyDecode statements into a common utils function. This cleans the code up a bit and makes use of the pymatgen-dependent orjson

[tschaume]

that does get called during the builders to populate all of the electrode pair entries in the *ElectrodeDoc.adj_pairs fields:

@tsmathis Would you prefer keeping the get_summary_dict() then? Or would you rewrite the builder to use the standard dict representation?

[tsmathis]

that does get called during the builders to populate all of the electrode pair entries in the *ElectrodeDoc.adj_pairs fields:

@tsmathis Would you prefer keeping the get_summary_dict() then? Or would you rewrite the builder to use the standard dict representation?

tbh, I don't fully understand either of the electrode models in terms of their data structure and the intended usage patterns. One example is those adj_pairs fields. That data is already represented in electrode_object.voltage_pairs, and a user could just call [pair.get_summary_dict() for pair in electrode_object.voltage_pairs]. Or it could be a computed_property decorator on the electrode models.

Both the conversion and insertion electrode models could use some scrutiny, but I think we would have to look at what queries are being run against those endpoints to see what users actually use

[esoteric-ephemera]

@tschaume tests are passing on my side with emmet-core==0.84.10. Should be OK to merge now, and we can work on improving the doc models in tandem

Would just suggest re-upgrading dependencies prior to merging to make sure there aren't lingering maggma-related dependencies - lmk when I should do that?