Skip to content
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

[loschmidt] Update analysis #291

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mpharrigan
Copy link
Collaborator

  • include job timestamp
  • re-think aggregation strategy

Background: the analysis code extracts the results into a pandas dataframe. You need to aggregate the quantities to make plots and create fits.

Previously: I knew additional columns (i.e. fields) would be added over time to the results dataframe. I thought I could be clever with the aggregation to make the addition of new fields automatic. This was done with the groupby_all_except function which inverts the api of groupby.

Then: we actually need a custom aggregation for the job_finished_time field. Every time the dataframe gets aggregated down, I've chosen to pick the "last" time of the group. You can imagine choosing the first or average. In any event: the groupby_all_except approach didn't make this magically work. One still needs to plumb through the new field with care.

This PR: make our groupby columns and the aggregation functions / output columns explicit everywhere. Going forward, this shouldn't behave badly if a new field is added. Although if you want to actually have the new field show up after using the aggregation/fitting functions you have to add it to the [...]_y_cols mapping in each relevant function.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Change-Id: Ib0ad42e19989ea513d31fcd406ce075d56c9a6b7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant