tomography / tomoscan

tomography scan script
https://tomoscan.readthedocs.io
Other
1 stars 11 forks source link

tomoscan.py and tomoscan_13bm.py do not obey Python case conventions #19

Closed MarkRivers closed 4 years ago

MarkRivers commented 4 years ago

When I wrote tomoscan.py and tomoscan_13bm.py I used mixed case for variable names and class method names. I used all lower-case for class names. This was based on my C++ experience.

I just read the Python style guide recommendations here: https://www.python.org/dev/peps/pep-0008/

This section says that class names should begin each word with an uppercase character. https://www.python.org/dev/peps/pep-0008/#class-names

So the classes should be TomoScan and TomoScan13BM, not tomoscan and tomoscan_13bm.

The exceptions should also begin each word with uppercase.

This section says to use lowercase with underscores for method names and class variables: https://www.python.org/dev/peps/pep-0008/#method-names-and-instance-variables

That means that all method names and class variables should be renamed.

Module names should be all lower-case, so tomoscan is fine for the module name.

It would probably be a good idea to follow these naming conventions, and to do this now before we start to use the software?

decarlof commented 4 years ago

There is also some confusion between the python library name tomoscan (same as the folder which is correct) and the file containing the tomoscan class (tomoscan.py).

Perhaps we should also consider calling the python library "scans" and rename the tomoscan folder as "scans", so we have scans/tomoscan.py

This way the import will be from scans import tomoscan which is nicer.

Let me play with the scan code as is at 2-BM and once I have it running there I will do a pull request with this and the above style convention.

decarlof commented 4 years ago

which also brings the question of where tomoScanApp and iocBoot should go. In python when there is other than python usually a 'source" folder is used. In this case we could do the following: set the library name to 'scans' crate a source folder then move inside:

source/scans/tomoscan/tomo2bm.py source/scans/tomoscan/tomo13bm.py ... source/scans/tomoScanApp/ source/scans/iocBoot

or

source/scans/tomoscan/tomoScanApp/ source/scans/tomoscan/iocBoot

so we are ready for adding other technique scans (?)

any demo should go in the docs (under demo or example) and contain the appropriate import from scans import tomo13.py this way it will not be be included in the api docs and only show in the example section.

Again I can propose these changes through a pull request after I run it at 2-BM

MarkRivers commented 4 years ago

I have made the changes I proposed above. Prior to these changes pylint was generating a very large number of warnings, and a very poor score. I have made many changes to make pylint happy. The remaining pylint messages are things I am happy with.

All variable, method, and class names have changed. I have updated the docs/ documentation to match these changes.

@decarlof you should use this version for 2-BM, don't spend time on the previous version because many names have changed.

MarkRivers commented 4 years ago

which also brings the question of where tomoScanApp and iocBoot should go

I think the standard EPICS module directories should stay where they are: tomoscan/configure tomoscan/tomoScanApp tomoscan/iocBoot

I am fine with changing that "tomoscan" repository name, but we should keep the standard EPICS module layout within the repository. I don't think we should change the repository name to "scans" however, because that is not specific enough to what it does.

It is fine to move the Python code to some better place in the repository.

decarlof commented 4 years ago

OK, will use the current version. In the meanwhile, I was able to get the tomoScan.template to run on a soft IOC I used for the old userInfo PVs but the way I know how to do this is too cumbersome (manually copy adl and db in the right place and edit the existing st.cmd etc., then fix the various macros). Can you point me to documentation on how to install an epics App properly? I looked at here but I am not sure is current.

MarkRivers commented 4 years ago

Can you point me to documentation on how to install an epics App properly?

The simplest thing to do to get running at 2-BM quickly is just to use the xxx application from APSshare, which should already be mounted on your Linux machine. I have pushed new versions of tomoscan/iocBoot/iocTomoScan/st.cmd and start_IOC. They each have commented out lines to run from APSshare rather than the locally built tomoScanApp for the executable and dbd files. I have tested running from APSshare with those and it works fine.

MarkRivers commented 4 years ago

If you want to build the tomoscan module, here is a basic recipe.

If you want to build EPICS base:

To build a minimal synApps:

I just did the above and it worked for me.

decarlof commented 4 years ago

@MarkRivers I could not get the simplest to work (cannot find tomoscan anywhere under APSshare (find /APSshare/epics/ -type d -name "iocTomoScan" returned nothing) but the second set of instructions is exactly what I was looking for and it worked perfectly. Thanks a lot.

MarkRivers commented 4 years ago

@decarlof I was not clear, you don't need tomoscan under APSshare, you just need xxx. You run from the tomoscan/iocBoot/iocTomoScan directory on your local system, but it gets the 2 needed files (xxx executable and iocxxxLinux.dbd) from APSshare.

The xxx executable is built with the 3 things you need:

The new versions of st.cmd and start_IOC should work fine to get those files from APSshare if you change what is commented out. Depending on where APSshare is mounted on your system you might need to change /APSshare to /mnt/APSshare or something like that.

Glad to hear my build instructions worked!

MarkRivers commented 4 years ago

The changes proposed here have been completed.