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

Use old curve setpoints for offsets #77

Merged
merged 2 commits into from
Nov 19, 2019
Merged

Use old curve setpoints for offsets #77

merged 2 commits into from
Nov 19, 2019

Conversation

ksunden
Copy link
Member

@ksunden ksunden commented Nov 15, 2019

Closes #52 Closes #69

@ghost
Copy link

ghost commented Nov 15, 2019

DeepCode Report (#424091)

DeepCode analyzed this pull request.
There are no new issues.

@ksunden ksunden added this to the 0.2.0 milestone Nov 15, 2019
@ksunden ksunden added the bug label Nov 15, 2019
@untzag
Copy link
Member

untzag commented Nov 19, 2019

@ksunden can you describe in paragraphs what this PR does?

@ksunden
Copy link
Member Author

ksunden commented Nov 19, 2019

The problem with the plotting is that the x-values of the offsets are incorrect.

Upon looking at the code again, I realize now that part of the problem is that the map_setpoints call occurs after the plotting.

The line being plotted is splined offsets from the input curve.
It was being plotted with the output curve (pre-remap) tune points, thereby showing the offsets as being offset from the correct value, instead of to the correct value

In cases with small offsets, this difference is hard to percieve.

If it had occurred prior to plotting, with the current arguments, the two arrays being switched here would be equivalent.

I will update this PR to move the map_setpoints call to before plotting, however I still wish to make the change already in this PR, as the ideas like in #60 and #68 would re-introduce this otherwise.

This is an example of something that is difficult to test against, as it is purely visual, no changes in the actual output.

@untzag
Copy link
Member

untzag commented Nov 19, 2019

thanks I understand now

@untzag untzag merged commit d0847d4 into master Nov 19, 2019
@untzag untzag deleted the tt_plot branch November 19, 2019 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Tune test plotting seems off Tune test raw_offsets should use old curve as x-axis
2 participants