-
Notifications
You must be signed in to change notification settings - Fork 1
Merge in dev for 0.3.0 Virtac release #11
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
Conversation
We set the default numpy printing options to be large so that large arrays are correctly printed to csv files without summarization or newlines.
Mirror.csv now has a scan column to allow users to configure a PV to process at one of the standard epics rates. This functionality essentially replaces the old refresh records which will be removed in a later commit
The 3 refresh PVs are not needed, instead we set the SCAN field for the 3 input PVs defined for the refresh PVs to be on a 1 second polling interval
Previously we were only getting PVs connected to lattice elements and not the lattice itself. I also added a check that the field is available in both the simulated lattice and live lattice as some of the lattices simulated fields do not have live equivalents to caget
This avoids duplicate PVs, as there are some pytac elements which share the same PV in the LIVE machine. This was harmless, but it is better not to have redundant data is it is confusing. Also small type hinting improvement
Also use the new scan column for both elements of the lattice and the lattice itself
Our selected print options are now only used within the with statement.
I split generate_pv_limits into two functions to allow us to spawn a cothread for each pytac element. This cothread will asyncronously do a caget to get the PV data for this element.
Due to current incompatibility with epicscorelibs. The default python version has also changed to a more conservative 3.10
Most functionality in this file is refactored code from virtac_main.
Functionality from masks and mirror_objects is now in pv.py. The mirror objects are now Inversion, Summation and Collation PV subclasses. The functionality in the masks is mostly in the RefreshPV
Instead of using a disable_emmitance flag with default false, we use enable_emittance with default true, as I think this makes more sense. I have also introduced an equivelant for tunefb and the tunefb setup is not initiated from within VirtacServer rather than __main__.py
This large commit required: -Importing the new pv classes -Creating softioc records using the new pv classes, softioc.builder is no longer required in virtac_server -Refactoring/improving the handling of some of the csv configuration data -Refactoring the method which configures "normal" pvs. This has been replaced by 3 functions, one for making lattice pvs, one for element pvs and one to call these two methods -Removal of the 9 VirtacServer owned dict/lists which stored the various softioc record types, there is now two dicts, one which holds all PVs and one which holds the read only PVs -Refactoring of _create_mirror_records, required to use the PV classes, some of this code has moved to pv.py
monitor_mirrored_pvs no longer needs to be called explicitly, monitoring of mirror pvs and mask pvs is now done automatically. With the mirror functionality done by CollationPV, InversionPV and SummationPV. The tunefb monitoring which was done through masks.py is now done with the RefreshPV subclass. However the user can explicitly enable/disable monitoring which enables/disables all of this behaviour after init.
This is called at init to give more context instead of the old print statement
We require atip>=0.2 as this is when we split atip/virtac atip 0.2 requires python3.11, so I bumped that too
MonitorPV now only takes a list of strings to initialise PV monitoring. The classes which inherit MonitorPV still take a PV as their arguments, but pass pv.name to the super() call. This makes the MonitorPV class a little cleaner.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11 +/- ##
==========================================
+ Coverage 19.43% 26.21% +6.77%
==========================================
Files 6 5 -1
Lines 499 599 +100
==========================================
+ Hits 97 157 +60
- Misses 402 442 +40 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Mostly adding -> None for functions which dont return anything
For improved readability
This replaces the more generic Callable type which was previously expected when passing a callback function to camonitor
The data read from the csv files is not manipulated, it is only used to set epics record fields which require str type, so we ensure that is the case.
These are not needed as we now have proper type hinting. (assuming the type hints are followed)
This is to better adhere to type hinting rules
We were not setting the value if the index was None
-Updated to latest copier template -Switched on pyright strict checking and fixed as many issues as I could -Switched back to mypy which is now passing and checking all functions -Spotted and fixed a couple of minor bugs as well -Removed some code which is redundant assuming the type hinting is followed
MJGaughran
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple minor issues, but we can cover them in a separate PR if they're non-trivial.
src/virtac/pv.py
Outdated
|
|
||
|
|
||
| Attributes: | ||
| self.name (str): The name used to get both the PV and its softioc record. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, and elsewhere: is it possible to remove the duplicate type hints? I can't think of a good reason for keeping them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For args, I will remove the duplicate type hints as they are automatically added to the function line in the docs. For attributes, they wont be given a type hint unless I specify it in the docstring, so I will leave them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its a bit subjective this, so ill make some changes and then we can discuss how the docs look after. There is definitely too much fluff in the API docs atm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can type hint attributes in the class definition. This explains the difference with ClassVar for class variables: https://docs.python.org/3/library/typing.html#typing.ClassVar
If you only include the type hint, it isn't assigned in advance.
Also various minor docstring fixes including fixing returns docstrings.
This is to better handle cases where _record is None and shouldnt be, and also where it is not None when it should be. We need a if not None guard in the setter and create_softioc_record as the setter guard wont be checked until after the record has been made which is too late.
get_value and set_value are a bit clearer to avoid anyone thinking that get/set refer to the softioc record rather than the records value field
-Major refactor to PV creation and data management
-Reworked how many PVs work, how the simulation updates PVs and how the PVs update the simulation
-Reworked and updated the docs
-Type hinting and docs strings
-Modified the CSV files to support the refactor
-Streamlined the csv creation process.