search

pyvisa / pyvisa-sim

A PyVISA backend that simulates a large part of the "Virtual Instrument Software Architecture" (VISA_)
https://pyvisa-sim.readthedocs.io/en/latest/
MIT License
69 stars 39 forks source link

Implemented RANDOM values. #96

Closed MatteoTondelli closed 6 months ago

MatteoTondelli commented 6 months ago

This is an alternative of #63 pull request which will work both for properties and queries, as suggested by @MatthieuDartiailh. Additionally multiple values can be generated, this way result of a DMM scan can be simulated.

The syntax is this: RANDOM(min, max, num_of_results) {...}. The output type can only be numerical.

MatthieuDartiailh commented 6 months ago

This looks interesting !

Could you:

As a side note did you consider generating integer in addition to float ?

MatteoTondelli commented 6 months ago

Ok, I will fix the conflicts and add some documentation. Actually there's no need to generate integer because you can already convert the output with the string formatter {:d}.

MatthieuDartiailh commented 6 months ago

Would that work for sequences though ? Anyway I am thinking about binary block that likely require a different mechanism anyway.

MatteoTondelli commented 6 months ago

Yes, probably you're right: sequences have to be handled in a different way, I did not consider that... However, if you're planning to implement a binary block, maybe the output format will be handled in the next releases, thus no reason to implement within this PR I guess. Alternatively I was thinking about using the following random statements RAND_INT and RAND_FLOAT to generate integers or floats repectively. What do you prefere?

MatthieuDartiailh commented 6 months ago

To properly support sequences, we should be able to specify a separator (as a fourth argument ?). I think RANDOM is fine. We can complexify the parsing logic later to allow to select the type and separator. But let's land this first before over engineering things.

MatteoTondelli commented 6 months ago

I've just resolved the conflicts and added a few lines of documentation. I was not able to compile the documentation myself...

MatthieuDartiailh commented 6 months ago

Looking at the example I wonder if it would not make more sense to specify the RAMDOM directive within the {} to avoid conflict between a surrounding message and data generation. WDYT ?

MatteoTondelli commented 6 months ago

Yes, definetely cleaner. I will change it and redo the commit.

MatteoTondelli commented 6 months ago

Previous regex would have worked both for RANDOM directive inside and outside the curled brackets, now it's strictly searched inside.

MatthieuDartiailh commented 6 months ago

Thanks ! Could you add some tests ?

A bunch of linting failures are unrelated to your changes and I can take care of fixing them before merging if you want.

MatteoTondelli commented 6 months ago

I will try but I'm very inexperienced with testing... I will try my best.

MatthieuDartiailh commented 6 months ago

Let me know if you want me to take over. It may just take me some time to get to this.

MatteoTondelli commented 6 months ago

I've added some tests, had bit troubles handling the random.seed(). Not the cleanest code, but I think this is something. What do you think? Is this enough for testing?

MatthieuDartiailh commented 6 months ago

It looks good. I am just bothered by the fact that the validity of the query is checked only when it is used. But that is likely a large problem than this PR. Do you mind if I fix the linting part here so that you do not need to rebase after ?

MatteoTondelli commented 6 months ago

Yes, not the strongest implementation from this side. Of course, you can fix the linting.

MatthieuDartiailh commented 6 months ago

I created #97 to address the linting. Once it goes in I will rebase your branch

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 86.00%. Comparing base (6b79f5b) to head (16d0f36).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #96 +/- ## ========================================== + Coverage 85.48% 86.00% +0.52% ========================================== Files 18 18 Lines 1040 1072 +32 Branches 152 159 +7 ========================================== + Hits 889 922 +33 + Misses 116 115 -1 Partials 35 35 ``` | [Flag](https://app.codecov.io/gh/pyvisa/pyvisa-sim/pull/96/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyvisa) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/pyvisa/pyvisa-sim/pull/96/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyvisa) | `86.00% <100.00%> (+0.52%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyvisa#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

MatthieuDartiailh commented 6 months ago

Thanks for your contribution !