To export non-log10 data when plot (X/Y) Axis Spacing is set to Log10… #44
Conversation
…, I check if either of these are set and if so, I undo the Log10 when exporting. Actual data in _plotSeriesCollection is not modified.
|
Not sure if I understand your comment. I do not modify _plotSeriesCollection (and so property PlotSeriesCollection), it actually gets modified when a user selects (X/Y) Axis Spacing to Log10 (which is causing the error). So what I do is undo that log while writing to file. Take a look at the code modification on this PR. Or did I misunderstand your comment? |
|
I understand - wanted to know feasibility of the plot VM holding on to the source data, unmodified, such that you can directly access that. And then the "projection" data for visualization purposes changes, depending on the toggle, but doesn't affect other parts of the system. Seems like something like that would be better than coupling the view and the serialization format. |
|
I completely agree. When I first looked at this, I assumed that somewhere the base data is stored and could be used. Should we fix this? |
|
Seems like a good idea to me, but I haven't reviewed the source code for quite some time. @lmalenfant what do you think? |
|
@hayakawa16 is correct, the right hand plot modifications do not change the underlying data they make the change for plotting only. I believe the base data is stored otherwise we wouldn't be able to keep modifying the plot. |
|
I just stepped through the code to confirm that the base data held in PlotSeriesCollection gets overwritten with Log10 values if Log10 (X/Y) axis spacing is selected. This is done in the very huge method ConstructPlot in PlotViewModel. On Line 938 the Log10 values get added to lineSeriesA and to tempPointArrayA. At this point DataPointCollection is still in non-log form. But then on line 976 tempPointArrayA is added to PlotSeriesCollection. Now PlotSeriesCollection has the data in Log10 form and has overwritten the non-log values. I've been thinking about how to clean this up and it is kind of related to the other WPF issue regarding the derivatives of complex reflectance amplitude and phase. Lisa hit on this when she tried to analyze the derivative issue. There is a physical separation between the left and right panels of the GUI and a intended processing separation in that the left side produces the data to be plotted, the right side manipulates that data, however some of our processing crosses over. For example, the left side calculates reflectance, the right side plots it, and if Log10 spacing is selected the PlotViewModel (right side) should only manipulate the data but not destroy the base calculation as it is being done in this issue. Similarly, the Complex toggles should only manipulate the underlying real and imaginary data to phase and amplitude. However, the derivatives (on left side) need to know what is selected on right hand side (real/imag, phase, amplitude) in order to determine the correct derivative. Lisa's idea is to put the Complex toggle on the left hand side panel and have it appear when a complex solution domain is selected. I'm not sure the best way to solve this. If we do perform an overhaul, I just ran code coverage and ForwardSolverViewModel has about 61% coverage and PlotViewModel has about 89% coverage. We might want to beef up FSViewModel coverage before we begin a major code modification. |
|
Good analysis, Carole! Yes, the original design was for the plot panel to be agnostic of what it was showing. But it's ok to have that change - perhaps we can introduce a more direct dependency "behind the scenes" so we don't have to move anything around. I'll take a look in the next 1-2 days and see if I have any suggestions. |
|
That would be great! In the meantime I plan to write a unit test for this result change and review the other unit tests we have in place. |
…ed Plot_ExportDataToText_Executed to export that data rather than PlotSeriesCollection. Added comments to these two variables for future reference.
|
@lmalenfant just pointed out to me the variable that houses the raw data, DataSeriesCollection. I modified Plot_ExportDataToText_Executed to use this instead of PlotSeriesCollection and it appears to work. I pushed this fix so all can see. So my comments above concerning this issue are wrong! |
|
I'm still working on this fix. Thought I'd fix it but may need lmalenfant's help. In the meantime, I'd like verify the user story. Today users @janakarana and @vasangithub told me how they envisioned the Export Data button would save data and I just wanted to make sure I have it correctly. Modify the "Export Data" button to "Export Raw Data". Then even if the user selects (X/Y) Axis Spacing to Log10, the non-log data will be written to file. To be consistent, for complex reflectance results, if the user selects Complex Phase or Amplitude, the real and imaginary data will be written to file, and if the Normalization is set to Max or Curve, these effects on the shown plot will not modify the data written to file. Users, please let me know if you agree. Thank you. |
…ls because too long). Added comment to IDataPoint indicating only X because is used for both Double and Complex data points. Updated ExportDataToText method to handle double and complex plot points. Now onto unit tests.
…wModelTests to show just edits I made.
… I copied master version of this file to branch folder and started editing. This way file changes are just what I did and do not include what VS did without my control.
…sily distiguish which is which. Attempted to write unit test but not sure I can Mock (NSubstitute) the SaveFileDialog.
|
I've been struggling to try to write a unit test for PlotViewModel.Plot_ExportDataToText_Executed(object sender, ExecutedRoutedEventArgs). The problem is that this method instantiates SaveFileDialog and then opens a window for user input with ShowDialog. I found this: I see a prior comment in the unit test that says this method is not tested because a Dialog window is brought up. I just thought things might have advanced. Any suggestions or clarifications that I can't indeed unit test this are welcome. |
…knowledge of the fixed independent variable is provided.
Hey Carole - you've found a very common pattern with code that wasn't designed for testability in mind. (My fault :). The correct thing to do is what is proposed in the post - create an abstraction for the concrete types that need to be mocked/faked. In other words, we could create wrapper classes that implement those interfaces. And then under the hood, those classes can use the concrete Windows file dialog classes in their underlying implementation. The biggest hurdle is that we'd need to update the constructor of |
|
@dcuccia, thank you so very much for your reply! I think I understand what you are suggesting, and it makes sense! I can give it a try again. |
|
No problem! Good luck! |
…tViewModel constructor that takes in IOpenFileDialog and IFileSystem, interfaces defined in class. Two unit tests pass but not sure of anything I did! Something is working though so I thought I'd push what I did.
|
Thanks to @dcuccia comments today, I got something to work!! I'm not sure of anything I did, but I wrote 2 unit tests and they appear to be working. I pushed my changes but might not get back to this until the new year. |
|
I should mention, there is another unit test that I did not touch, PlotViewModelTests.Verify_max_normalization(). When run in a batch, this test fails in Test Explorer and in Resharper run unit tests, however if I run just alone afterward, it passes. So something in the ordering of the test execution is affecting this test's success. |
|
Awesome! I'll take a look when I can! |
…ort-actual-y-axis-values-when-log-10-y-axis-spacing-is-selected
|
Hi @hayakawa16 I started to look at these changes and I found the issue with the failing unit test (I fixed it locally), your new test redefines _plotData and invalidates it for subsequent tests so I changed it to a local variable. When I changed this, another unit test was failing for the same issue (possibly it passed previously due to the order of the tests), I fixed this one also. I'll review the class changes now and post any addition comments on this thread. |
|
Thank you @lmalenfant for your review! Please feel free to push an updates to the branch. |
|
@hayakawa16 When I try to save the raw data, I get a null reference exception, maybe we can look at this together next week. Steps to reproduce: |
…d removed the redundant code
|
|
@hayakawa16 we already had a draft PR for this branch and because you created it, I cannot add you as a reviewer. I would still like your review and then I can approve and merge. I still plan to do some more testing but initial tests show that it is working well. I realized after I committed the latest changes, that we should probably implement this for the MapViewModel also. This can be a part of the increased code coverage though. |
|
This looks terrific! I ran some examples with two sets of plots and checked output and all looks good. Thanks for figuring out how to do this!! |
lmalenfant
deleted the
bugfix/43-export-data-feature-does-not-export-actual-y-axis-values-when-log-10-y-axis-spacing-is-selected
branch
To export non-log10 data when plot (X/Y) Axis Spacing is set to Log10, I check if either of these are set and if so, I undo the Log10 when exporting. Actual data in _plotSeriesCollection is not modified.
I am creating a draft PR for now.