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

SourceData interface #206

Merged
merged 12 commits into from Aug 25, 2021
Merged

SourceData interface #206

merged 12 commits into from Aug 25, 2021

Conversation

takluyver
Copy link
Member

An object representing one source, with several keys:

xgm = run['SPB_XTD9_XGM/DOOCS/MAIN']

'beamPosition.ixPos.value' in xgm  # Check for key
xgm_keys = xgm.keys()  # Find all (selected) keys
beam_x = xgm['beamPosition.ixPos.value']  # Get a KeyData object

Closes #55

@takluyver takluyver added the enhancement New feature or request label Aug 2, 2021
@philsmt
Copy link
Contributor

philsmt commented Aug 4, 2021

Thank you, loving this interface pattern. I'm definitely using KeyData much more than the classic interface already.

It's not strictly touch the code changed in this PR, but I feel the specific syntax here makes it even more natural to expect keys to work without the attached .value:

'beamPosition.ixPos.value' in xgm # Check for key

I realize this remains debatable whether a Karabo viewpoint is valid on files. I actually meant to discuss at some point a general strategy on how to handle this conundrum, but it creeps up here again. I understand if you prefer to postpone it as part of a bigger problem, though.

@takluyver takluyver added this to the 1.8 milestone Aug 5, 2021
@takluyver
Copy link
Member Author

Thanks; I think it makes sense to try to do something about .value here.

@takluyver
Copy link
Member Author

OK, both key in sourcedata and sourcedata[key] should now allow control keys without the .value suffix.

Since run[source, key] and various methods like run.get_array() now go through the sourcedata object, this should indirectly mean that you can leave off .value in a whole lot of places. We still treat the name with .value as canonical, though, for e.g. sourcedata.keys().

@tmichela
Copy link
Member

tmichela commented Aug 5, 2021

Nice 👍

can we make a quick mention in the documentation, maybe here?

@takluyver
Copy link
Member Author

Yes, of course, good point!

@tmichela
Copy link
Member

LGTM

inc_suspect_trains=self.inc_suspect_trains,
)

def keys(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry striking the same notch (this saying probably doesn't work in English), but what do you think about adding an optional flag here to collapse .value and .timestamp? Something like omit_suffix or collapse_value_and_timestamp.

While it might only be useful in generic programming, I could imagine it something people want to do with the output from this function, and it might be convenient to provide it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems reasonable enough. What about something that defaults to True for including both as separate keys, like inc_timestamps=False or value_and_timestamp=False?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, either way is fine with me!

@takluyver
Copy link
Member Author

I've implemented it with inc_timestamps=False as the option to hide the timestamp/value suffixes. I'm not 100% convinced on that name if anyone has a better idea, though. It's ignored for instrument sources.

I've also used this option when listing keys in lsxfel, so you no longer see separate timestamp & value listed there.

@philsmt
Copy link
Contributor

philsmt commented Aug 23, 2021

Thanks, LGTM!

@takluyver takluyver merged commit 3c77484 into master Aug 25, 2021
@takluyver takluyver deleted the sourcedata branch August 25, 2021 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Objects for individual sources or datasets?
3 participants