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

Renaming Run to Shot #85

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

philipstarkey
Copy link
Member

@philipstarkey philipstarkey commented Nov 5, 2020

Resolves #81

Renamed classes/arguments/docs from run to shot and added dprecation notices where appropriate.

Arguments (distinct from keyword arguments) were renamed without worrying about backwards compatibility under the assumption that people are not using them as keyword arguments (which is technically possible).

Resolves Rename Run to Shot labscript-suite#81

Renamed classes/arguments/docs from run to shot and added dprecation notices where appropriate.

Arguments (distinct from keyword arguments) were renamed without worrying about backwards compatibility under the assumption that people are not using them as keyword arguments (which is technically possible).
@philipstarkey
Copy link
Member Author

@zakv Would also appreciate your eyes on this (don't seem to be able to explicitly request a review from you using the GitHub interface since you are not a project member. Might have to fix that)

Copy link
Contributor

@zakv zakv left a comment

Choose a reason for hiding this comment

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

Overall looks good! I made a few comments, and have a couple of overall questions.

  1. Is there any particular reason for using double underscores rather than single? It's not clear to me how the name mangling helps.
  2. Why is "Shot" better than "Run"? I agree that normally I'd use the word "shot" (in fact our lab uses a custom Shot class than inherits from lyse.Run) but I would think that "run" would mean the same thing. Is the word "run" misleading somehow? I agree that if lyse were being written from scratch I'd rather name this class Shot than Run but I'm not 100% convinced it's confusing enough to be worth making users update all of their analysis scripts and other code.

lyse/__init__.py Outdated Show resolved Hide resolved
lyse/__init__.py Show resolved Hide resolved
lyse/__init__.py Outdated Show resolved Hide resolved
@zakv
Copy link
Contributor

zakv commented Nov 5, 2020

Also, Examples.rst needs to be updated to use Shot instead of Run, and introduction.rst needs one line updated ("the entire dataset from all the runs that are currently loaded").

Edit: introduction.rst, not index.rst, needs that change.

* split deprecation messages across multiple lines
* Fixed deprecation message punctuation
* Fixed missing property decorator in `Sequence` class
* Updated docs to use `Shot` class.
@philipstarkey
Copy link
Member Author

Overall looks good! I made a few comments, and have a couple of overall questions.

  1. Is there any particular reason for using double underscores rather than single? It's not clear to me how the name mangling helps.

In this instance I did it to made the variable read only. Not strictly necessary, but there it's really an internal variable and it's better practice to keep it from being modified outside of the class. It's basically just defensive programming, and makes it easier for internal changes to be made down the line without worrying about whether we're breaking the API.

  1. Why is "Shot" better than "Run"? I agree that normally I'd use the word "shot" (in fact our lab uses a custom Shot class than inherits from lyse.Run) but I would think that "run" would mean the same thing. Is the word "run" misleading somehow? I agree that if lyse were being written from scratch I'd rather name this class Shot than Run but I'm not 100% convinced it's confusing enough to be worth making users update all of their analysis scripts and other code.

Run could be interpreted as a set of shots (a "run" of shots"). Run was very early nomenclature we adopted, but it rapidly become confusing when used in every day speech. We thus settled towards the more obvious "Shot" and "Sequence" which cannot be confused. We were hesitant to change the API at the time, but I think it's probably time to bite the bullet and do it. I agree it's a bit of a pain to change code, but lyse.Run is not going anywhere for a long time (can't imagine lyse 4.0 is close given 3.0 was only just released) and it's a 5 second fix per script if you want the deprecation warning to go away. I could maybe make it so the deprecation warning only showed up once on the first shot and not subsequent shots? But that risks being missed if users don't restart lyse often.

@zakv
Copy link
Contributor

zakv commented Nov 6, 2020

It's basically just defensive programming, and makes it easier for internal changes to be made down the line without worrying about whether we're breaking the API.

Good to know, thanks! I wasn't sure if there was something that the double underscore name mangling signaled to users that a single underscore didn't.

We thus settled towards the more obvious "Shot" and "Sequence" which cannot be confused

To be honest our lab used to use "shot" and "sequence" interchangeably, haha. That was back when we used a labview-based control system where you designed shots by listing a sequence of output times/voltages in a table. I suspect though that the large majority of people would interpret "shot" and "sequence" in the way you mentioned.

All your changes look good from my perspective. The PR looks ready to merge as far as I can tell.

@dihm dihm mentioned this pull request Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename Run to Shot
2 participants