-
Notifications
You must be signed in to change notification settings - Fork 1
Hilary dev #53
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
Hilary dev #53
Conversation
…tely to pcpostprocess
Allow for protocols in export_config.py that are not included in the dataset
…a protocol in the experiment (should now skip those protocols)
…a protocol in the experiment (should now skip those protocols)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #53 +/- ##
=======================================
Coverage ? 78.79%
=======================================
Files ? 7
Lines ? 646
Branches ? 0
=======================================
Hits ? 509
Misses ? 137
Partials ? 0 ☔ View full report in Codecov by Sentry. |
|
I don't remember what this was, but perhaps I can generate some before and after plots to see if we like it? |
|
Seems like the changes are sensible. There a few commented outlines that should probably just be outright removed. Before and after plots would be nice, but maybe we only need to look at the "after" versions! |
|
|
||
| data_list = os.listdir(args.data_directory) | ||
| export_config.D2S_QC = {x: y for x, y in export_config.D2S_QC.items() if | ||
| any([x == '_'.join(z.split('_')[:-1]) for z in data_list])} |
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 think this should work fine. But this could throw an error if the files aren't set out properly. Maybe we should just check that there is at least one "_" in all of these files. I think it's mostly safe to assume that the data folders will be formatted correctly, but there's nothing stopping someone from dragging any other sort of file in there!
| label=r'$I_\mathrm{L}$.' f"g={b1:1E}, E={-b0/b1:.1e}") | ||
| # sortedy = sorted(before_currents[i, :]) | ||
| # ax.set_ylim(sortedy[30]*1.1, sortedy[-30]*1.1) | ||
|
|
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.
Should probably just get rid of these commented-out lines
pcpostprocess/subtraction_plots.py
Outdated
| label=r"$I_\mathrm{L}$." f"g={b1:1E}, E={-b0/b1:.1e}") | ||
| # sortedy = sorted(after_currents[i, :]) | ||
| # ax.set_ylim(sortedy[30]*1.1, sortedy[-30]*1.1) | ||
| # if ax.get_legend(): |
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.
More comments to remove
| ax.set_ylabel(r'leak-corrected traces') | ||
| first = False | ||
|
|
||
| # sortedy = sorted(corrected_after_currents+corrected_before_currents) |
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.
Comments to delete
| # ax.set_ylim(sortedy[60]*1.1, sortedy[-60]*1.1) | ||
| ax.legend() | ||
| # ax.tick_params(axis='y', rotation=90) | ||
| # ax.yaxis.set_major_formatter(mtick.FormatStrFormatter('%.1e')) |
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.
Comments to delete
|
|
||
| corr_dict = {'sweeps': sweeps, 'pcs': pcs} | ||
| return corr_dict | ||
|
|
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.
Duplicated code from leak_correct.py?
We could remove the other file, but I don't think it's been deleted by these commits
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.
Yeah, I don't think we should be fitting the leak again with a new linear regression (just below here), we want to show the line we fitted elsewhere (in case we tweak the process there?)
joeyshuttleworth
left a comment
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.
Looks good. Should remove some commented-out lines and either delete or change leak_correct.py so we don't duplicate code
|
Not sure this was finished? @hilaryh how's it going? Did you find a decent ☕ ? |
well they definitely don't look nice with the legend all over the top of the interesting bits and cut off text! But it is nice to use more plot area for plots than whitespace... |
|
a bit more sharex and sharey and just showing tick marks instead of tick labels where they are shared would probably tidy it up a fair bit |
|
I feel like that may have been the commented out bit, @joeyshuttleworth ? |
Hi everyone! Decent coffee here. None hold a candle to Effy yet but I have high hopes for next Tuesday's pick. The plots are definitely not supposed to look like that! I may have forgotten to add subtraction_plots.py to my last commit... Will do now! |
…dded a tight_layout line to avoid overlap
|
I've fixed the layout problems (I hope!). The commented out code can all be deleted - the sortedy parts were from a brief attempt to cut out the capacitative spikes in some of the harder to see traces without going back to the remove_capacitative_spikes code but obv something a little more robust is necessary. |
|
Thanks @hilaryh ! |
mirams
left a comment
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.
Could you paste up what the subtraction plot looks like now?
|
|
||
| corr_dict = {'sweeps': sweeps, 'pcs': pcs} | ||
| return corr_dict | ||
|
|
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.
Yeah, I don't think we should be fitting the leak again with a new linear regression (just below here), we want to show the line we fitted elsewhere (in case we tweak the process there?)
|
OK will come back to this later |
| all_leak_params_after = [] | ||
| for i in range(len(sweeps)): | ||
| before_params, _ = fit_linear_leak(before_currents, voltages, times, | ||
| before_params, _ = fit_linear_leak(before_currents[i, :], voltages, times, |
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.
@frankiepe - Hilary may already have done some of this, just in this branch!


Description
Types of changes
Testing
Documentation checklist