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

Scans in Shared Python Scripts: Replace populate #5401

Closed
John-Holt-Tessella opened this issue Apr 28, 2020 · 4 comments
Closed

Scans in Shared Python Scripts: Replace populate #5401

John-Holt-Tessella opened this issue Apr 28, 2020 · 4 comments

Comments

@John-Holt-Tessella
Copy link
Contributor

John-Holt-Tessella commented Apr 28, 2020

ISISNeutronMuon/InstrumentScripts#60

As a developer I would recommend getting rid of populate. It can cause really weird effects if a block is the same name as a module e.g. time

Instead replace with a module which has a class member variable for each active block. On most beam lines this is imported as b.. b.block_name should return something that is interpreted as a string in scan(<motion> and cset and cget. Ideally it should update itself when the blocknames change.

Acceptance Criteria

  • A decision is made on populate and the decision is recorder. (must be agreed in general between scientists)
    • done they are happy to remove it if they can replace it with b.
  • Populate is removed
  • Documentation is updated
  • a blocks module is added that has a list of blocks variable in.
  • Tests for any code change are added

Notes

  • This is a breaking change so maybe we just shouldn't

    • we should because it is in the build_ins and can cause weird problems in the future
  • Its purpose is to create variables which can be used instead of block names so that scan can be written: scan(block, ... instead of scan("block", .... But it comes with caveats that when the user changes config they must remember to rerun populate to get new blocks and it is different to the recommended way of running g.cset i.e. cset("block", value). However I do not know the history and so would like to understand its motivation.

  • interpreted as a string: could mean just a return a string but maybe there is a way of returning a more complicated object that will evaluate to a string but have more complicated properties like units etc. This might eventually allow us to do b.blockname.scan(-0.5, 0.5, 1) or b.blockname.cset(10) or even b.blockname = 10.

@John-Holt-Tessella John-Holt-Tessella added this to Proposals in Planning via automation Apr 28, 2020
@FreddieAkeroyd
Copy link
Member

This is a hang over from VMS and not wanting to add quotes. I created global variables like this in Open GENIE for CSET and they were initialised when a user typed GETBLOCKS on the genie command line, they needed to type this for other reasons than just creating the variables. Whenever you modified blocks in SECI you were prompted to type "getblocks", if you changed config in SECI genie was killed so it would get reloaded too.

In genie python we didn't implement the global variables, so you don't need to type get_blocks(), but must use quotes. Given removing populate() is a breaking change, we should make the case to the scientists - I think it is worth making that case though.

@DominicOram DominicOram moved this from Proposals to High in Planning Apr 30, 2020
@DominicOram
Copy link
Contributor

@John-Holt-Tessella to change acceptance criteria to be implementation + tests

@DominicOram DominicOram added the 5 label Apr 30, 2020
@kjwoodsISIS kjwoodsISIS removed this from High in Planning Apr 30, 2020
@kjwoodsISIS kjwoodsISIS added this to the SPRINT_2020_04_30 milestone Apr 30, 2020
@John-Holt-Tessella John-Holt-Tessella changed the title Scans in Shared Python Scripts: Discussion - Remove populate Scans in Shared Python Scripts: Remove populate Apr 30, 2020
@John-Holt-Tessella
Copy link
Contributor Author

Larmor, Inter and Polref instrument scineitst were happy to remove populate but wanted to keep a way of defining blocks without typing strings. (Some issues had been found with populate in the past) @FreddieAkeroyd has suggested that we create a "b." modules which lists all the blocks as member variables. This should be doable and may be useful for other beamline too. So I am changing the acceptance criteria for this ticket to include that and will get feedback from the IBEX team

@John-Holt-Tessella John-Holt-Tessella moved this from Ready to In Progress in IBEX Project Board May 5, 2020
@John-Holt-Tessella John-Holt-Tessella changed the title Scans in Shared Python Scripts: Remove populate Scans in Shared Python Scripts: Replace populate May 5, 2020
@John-Holt-Tessella John-Holt-Tessella added this to In Progress in Reflectometry May 11, 2020
@John-Holt-Tessella John-Holt-Tessella moved this from In Progress to Review in IBEX Project Board May 11, 2020
@John-Holt-Tessella John-Holt-Tessella moved this from In Progress to Review in Reflectometry May 11, 2020
@DominicOram DominicOram moved this from Review to Ready in IBEX Project Board May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants