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

Re-write Topas curve IO #39

Merged
merged 11 commits into from
Feb 26, 2019
Merged

Re-write Topas curve IO #39

merged 11 commits into from
Feb 26, 2019

Conversation

ksunden
Copy link
Member

@ksunden ksunden commented Feb 18, 2019

May not be done, not really sure, could use a set of fresh eyes at the very least (either my own after sleep or someone elses)

Anywho, Differences introduced herein (that I can remember off the top of my head:

  • No longer uses "kind" as a given parameter with meaning, instead getting that from the second line of the file

    • No more "TOPAS-C" or "TOPAS-800", instead this will be "MIXER{n}" or "OPA" or "NOPA", the latter is determined by the third line in the file if the second is "OPA/NOPA"
  • No meaning to the order/number of files given, all curves in the files given are read in and linked together by the new read_all classmethod. This is used by read

    • An internal function _read_file is used to read a particular file, but does not link it to it's subcurve (it does fill out the source_setpoints, though
  • Extra information encoded into the curve objects. There is currently no good way to enforce that this information is present if somebody tries to make a "TopasCurve" from scratch. This is not reccomended usage.

  • saving does not keep all files, the full kwarg does nothing at this point.

    • This is less than ideal, but want to discuss semantics of full.
    • I do not track old_filepaths anymore, as all information to write the crv is in the object
    • Old behavior did not propagate changes to subcurves in memory.
  • Since it's not reading the old_filepaths, any curves that are in the same file are no longer there...

    • Sig/Idl are recomputed so they are always there together
    • We may want to keep this, as it makes the files more portable/easier to swap between interactions
    • requires putting old_filepaths back and doing some fancyness.
    • Could also have a kwarg like add_cuves and accept an iterable or a file path, or True to look for the old file. Would not be too hard, and I tried to program with something like this in mind, but did take a shortcut or two that would have to be a bit more flexible for this approach.
  • I would like to use the fmt from the curves/dependents themselves, but that system is not yet where I want it

  • Name field is different, but only becouse I was lazy/not sure if file name is what we want to use for the name field.

  • Uses np.DataSource for all that remote/compressed file goodness like from_ methods in WT

  • Motor Names are not in these files... and I didn't want to keep using hard coded names

    • They are in the ini files, which we could parse, but I'm hesitant to require something extra
    • We could use the "comment" field in crv files to put tidy_headers headers. I'm fairly confident it won't affect actual usage in WinTopas, though it may affect some UI elements when switching curves.
    • Currently using the string representation of the motor index, as that is uniquely identifying, but it is unsatisfactory and not very human-friendly

@ksunden
Copy link
Member Author

ksunden commented Feb 18, 2019

The choice of "name" is particularly bad due to the way the new file name is chosen, which tries to strip the old timestamp from the name, as it was previously the file name.

@ksunden
Copy link
Member Author

ksunden commented Feb 18, 2019

Also, since files have multiple interactions, it seems poor to name it off of the interaction string

@ksunden
Copy link
Member Author

ksunden commented Feb 18, 2019

Windows is being annoying... Something about a PermissionError... I'll figure it out in the morning

@ksunden
Copy link
Member Author

ksunden commented Feb 18, 2019

This pull request introduces 3 alerts when merging 4fb3942 into b6ddfbe - view on LGTM.com

new alerts:

  • 3 for Unused import

Comment posted by LGTM.com

@ksunden
Copy link
Member Author

ksunden commented Feb 18, 2019

Okay, looks like the permission error was only as a result of __exit__ on the context manager for the tempdir being called before the file was closed, Which a) needs a call of f.close prior to returning in _read_file, but b) was failing anyway on windows for some reason due to trying to interpret "MIXER2" as an int...

@p770193 any chance you could pull it down and run tests locally on your windows box? I can also use the lab machines if needed.

@ksunden
Copy link
Member Author

ksunden commented Feb 19, 2019

Updates to lists of differences above:

full kwarg on save will now save all files in the "family", meaning all "siblings" "subcurves" and "supercurves" which are in memory. Note: Unlike past behavior, if changes have occured to those curves in memory, they will be propegated, if multiple curves have the same interaction string that are accessible via this tree, that is undefined behavior, and either may be chosen.

The default "Name" is now the same as the "Kind", meaning it is OPA or MIXER{n}, again, not sold on these names, but wanted something easyto get where I wanted it.

@ksunden ksunden merged commit 6ef069e into master Feb 26, 2019
@ksunden ksunden deleted the topas_read branch February 26, 2019 00:30
@untzag untzag added this to the 0.1.0 milestone Aug 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants