-
Notifications
You must be signed in to change notification settings - Fork 4
Phonons analysis app #223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Phonons analysis app #223
Conversation
| cumulative_dist, i = 0.0, 0 | ||
| connections = [True] + connections | ||
|
|
||
| for seg_dist, connected in zip(distances, connections, strict=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With strict=False this will stop when either of the two lists ends, even if the other is longer. Is this what you want?
| return Div("Click on a metric to view the structure.") | ||
|
|
||
|
|
||
| def scatter_and_assets_from_table( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't this by handed by something like plot_from_table_cell?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it also keeps the model data so that we can access model-specific assets from the scatter plot. (with weas, we dont take model-specific assets as the structures are all the same (right?))
| return content, meta, active_cell | ||
|
|
||
|
|
||
| def model_asset_from_scatter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does the model-specific part come in?
As above, I also wonder if asset it the most useful description? Is this actually loading from /assets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The model context is carried via scatter_metadata: when the user clicks a table cell, scatter_and_assets_from_table stores the active model, metric, and the list of points.
asset is just a general way of saying "secondary view component" e.g. scatter -> asset e.g. scatter -> structure or png or other. i was trying to make a general. (wouldnt use for strucutures here as model independent, but maybe if we had specific relaxed structures which were model dependent then we could use this for that too)
| return plot_parity_decorator | ||
|
|
||
|
|
||
| def cell_to_scatter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to make this an option for the exist scatter decorator, rather than a separate one? I think basically all the logic is the same, it's just whether we combine the traces, right? We may even want to do individual + combined increasingly e.g. when scaling starts to get messy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we build these as part of the app, rather than saving them to json and loading them as we normally do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
save on analysis time
ml_peg/app/utils/plot_helpers.py
Outdated
| Parameters | ||
| ---------- | ||
| points | ||
| Sequence of metadata dictionaries containing reference/prediction values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit confusing because it makes it sound like data rather than metadata, but I think it's because we save various things (including but not limited to the x/y values) of each point in these dicts? Probably worth a making the connection to "points" in this docstring too, since out of context it's quite confusing
…, generalise non-specific helpers to plot_helpers.py and clean up
Co-authored-by: Elliott Kasoar <[email protected]>
Co-authored-by: Elliott Kasoar <[email protected]>
831bf6e to
7e2117a
Compare
|
Issue to fix: when rows are reordered, the interactive cells show the original model in that position |
|
So i've gotten to the bottom of the band mismatches.
|
| __pycache__/ | ||
| ml_peg/app/data/ | ||
| ml_peg/app/data/* | ||
| !ml_peg/app/data/onboarding/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's a tiny change, but can we do this separately e.g. its own PR? It's unrelated to phonons
So a fix that works for some test samples is instead of auto, just calculating the primitive matrix: We probably need to rerun some phonons, but using these larger cells we used before doenst effect the max or min freqs etc, mainly the BZ MAE |
Pre-review checklist for PR author
PR author must check the checkboxes below when creating the PR.
Summary
phonon analysis and app, without the calc for the moment.
Linked issue
Resolves #222
Progress
Testing
mattersim and mace-omat-0
New decorators/callbacks
decorators:
callbacks:
- scatter_and_assets_from_table
- model_asset_from_scatter
i think theres potential to combine these with other callbacks so lets dicuss? e.g. make table -> scatter and scatter to structure more general (e.g. scatter -> any asset). but maybe we want to keep it separate so its more simple for users