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

Feature / rescale ticks and units in plot_by_id #1239

Merged
merged 18 commits into from
Aug 22, 2018

Conversation

astafan8
Copy link
Contributor

@astafan8 astafan8 commented Aug 20, 2018

Request along with #1200. This brings the "rescale_axis" feature of plotting functionality of the old dataset to the plot_by_id of the new dataset. The feature scale the ticks and the units (on the axes labels) to engineering units (k, M, m, n, etc.) so that the plots are more readable. For example, 2.000.000.000 in V becomes 2 in GV if all the numbers on this axis are not larger than 1.000.000.000.000 volts. The feature can be turned of by using a corresponding kwarg.

ToDo:

  • use max of abs value
  • more tests
  • update "offline plotting tutorial"
  • improve docstrings in dataset.plotting module for private functions

unit = data_dict['unit']

if unit in _SI_UNITS:
maxval = np.nanmax(data_dict['data'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be the max of the absolute value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think it should. but the original implementation did not have it :) I'll change it and add a test for it.

@astafan8 astafan8 self-assigned this Aug 21, 2018
@astafan8 astafan8 changed the title Feature / rescale ticks and units in plot_by_id [WIP] Feature / rescale ticks and units in plot_by_id Aug 21, 2018
@codecov
Copy link

codecov bot commented Aug 21, 2018

Codecov Report

Merging #1239 into master will decrease coverage by 0.96%.
The diff coverage is 53.03%.

@@            Coverage Diff             @@
##           master    #1239      +/-   ##
==========================================
- Coverage   80.15%   79.18%   -0.97%     
==========================================
  Files          51       52       +1     
  Lines        7053     7201     +148     
==========================================
+ Hits         5653     5702      +49     
- Misses       1400     1499      +99

@astafan8
Copy link
Contributor Author

astafan8 commented Aug 21, 2018

@QCoDeS/core and @ThorvaldLarsen: should the rescaling of units and ticks be enabled by default or not?

@astafan8
Copy link
Contributor Author

@QCoDeS/core and @ThorvaldLarsen Another question: should seconds be rescaled? :) ks (kiloseconds) looks weird :)

@ThorvaldLarsen
Copy link
Contributor

Definitely. We rarely have ks but use ns or us (microsecond) all the time. Thanks!

@astafan8 astafan8 changed the title [WIP] Feature / rescale ticks and units in plot_by_id Feature / rescale ticks and units in plot_by_id Aug 21, 2018
@astafan8
Copy link
Contributor Author

@QCoDeS/core ready for review.

Copy link
Contributor

@WilliamHPNielsen WilliamHPNielsen left a comment

Choose a reason for hiding this comment

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

I think that type hints should be added to all the functions introduced here.

@astafan8
Copy link
Contributor Author

@WilliamHPNielsen added type annotations. ready for merge now?

@astafan8 astafan8 merged commit c190d9f into microsoft:master Aug 22, 2018
@astafan8 astafan8 deleted the feature/rescale_axes_in_plot_by_id branch August 22, 2018 10:58
giulioungaretti pushed a commit that referenced this pull request Aug 22, 2018
Merge: f138bee 162dfcd
Author: Mikhail Astafev <astafan8@gmail.com>

    Merge pull request #1239 from astafan8/feature/rescale_axes_in_plot_by_id
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants