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

Add support for XDF files #11

Merged
merged 62 commits into from Feb 16, 2017

Conversation

Projects
None yet
2 participants
@Yida-Lin
Contributor

Yida-Lin commented Nov 19, 2016

  • Initialize streams with NaN instead of zeros
  • Fix #3
Show outdated Hide outdated src/src.pro Outdated
@Yida-Lin

This comment has been minimized.

Show comment
Hide comment
@Yida-Lin

Yida-Lin Dec 25, 2016

Contributor

Merry Christmas! Improved the channel dialog and several other dialogs:

2016-12-24 2

I need some advice on coloring streams. Currently the save/load color functions are getting in my way; the color setting of the previous file is saved, but I don't need them, because the stream number and channel number of each file is different. I wish to choose color every time when loading based on how many streams there are, rather than loading the setting of the previous file.

Also I propose should we add a designer form for the main window for easier editing/designing? I was also thinking about to add some hotkeys, like Ctrl+O opening the file etc., and with a designer form this can be more easily done.

Update 12/26:

I wrote a function called subtractByMean().

image

and below is a comparison of before and after calling the funtion:

image

image

Update 12/27:

The meta-info is now showing in the channel label:

image

Contributor

Yida-Lin commented Dec 25, 2016

Merry Christmas! Improved the channel dialog and several other dialogs:

2016-12-24 2

I need some advice on coloring streams. Currently the save/load color functions are getting in my way; the color setting of the previous file is saved, but I don't need them, because the stream number and channel number of each file is different. I wish to choose color every time when loading based on how many streams there are, rather than loading the setting of the previous file.

Also I propose should we add a designer form for the main window for easier editing/designing? I was also thinking about to add some hotkeys, like Ctrl+O opening the file etc., and with a designer form this can be more easily done.

Update 12/26:

I wrote a function called subtractByMean().

image

and below is a comparison of before and after calling the funtion:

image

image

Update 12/27:

The meta-info is now showing in the channel label:

image

@cbrnr

This comment has been minimized.

Show comment
Hide comment
@cbrnr

cbrnr Dec 28, 2016

Owner

I love the subtractByMean function! Does this depend on XDF somehow? If not, please create a separate PR just for this functionality. That way, I can merge it much more quickly into master.

Minor comment: Maybe the function should be called subtractMean instead of subtractByMean?

Owner

cbrnr commented Dec 28, 2016

I love the subtractByMean function! Does this depend on XDF somehow? If not, please create a separate PR just for this functionality. That way, I can merge it much more quickly into master.

Minor comment: Maybe the function should be called subtractMean instead of subtractByMean?

@cbrnr

This comment has been minimized.

Show comment
Hide comment
@cbrnr

cbrnr Dec 28, 2016

Owner

Regarding the form for the main window, if it's just because you want to add shortcuts I'd say this is not necessary. Qt automatically assigns platform-dependent shortcuts for certain actions such as Open if you assign it the correct action.

Owner

cbrnr commented Dec 28, 2016

Regarding the form for the main window, if it's just because you want to add shortcuts I'd say this is not necessary. Qt automatically assigns platform-dependent shortcuts for certain actions such as Open if you assign it the correct action.

@Yida-Lin

This comment has been minimized.

Show comment
Hide comment
@Yida-Lin

Yida-Lin Dec 28, 2016

Contributor

@cbrnr Thank you! Currently this function is for XDF only. If you would like, I probably can imitate one for other formats as well, though I need to think where to put it, and where to store the offset information.

Yes I agree: let's modify it as subtractMean().

You are right: the Ctrl+O already works! Thanks for that.

Contributor

Yida-Lin commented Dec 28, 2016

@cbrnr Thank you! Currently this function is for XDF only. If you would like, I probably can imitate one for other formats as well, though I need to think where to put it, and where to store the offset information.

Yes I agree: let's modify it as subtractMean().

You are right: the Ctrl+O already works! Thanks for that.

@cbrnr

This comment has been minimized.

Show comment
Hide comment
@cbrnr

cbrnr Dec 30, 2016

Owner

I think we should definitely implement subtractMean in a generic way! I propose that we store a vector of means for all channels when loading a file. This can be done within the min/max search. Then we could have an action called "Remove offset" in the View menu, which when triggered, just subtracts the stored means from the channels.

Owner

cbrnr commented Dec 30, 2016

I think we should definitely implement subtractMean in a generic way! I propose that we store a vector of means for all channels when loading a file. This can be done within the min/max search. Then we could have an action called "Remove offset" in the View menu, which when triggered, just subtracts the stored means from the channels.

@Yida-Lin

This comment has been minimized.

Show comment
Hide comment
@Yida-Lin

Yida-Lin Dec 30, 2016

Contributor

@cbrnr Great idea! I am currently working on the documentation of libxdf using doxygen. Once I finish I will work on the generic subtractMean().

image

Update 12/30:

The documentation of libxdf has been uploaded to the shared google drive folder and will keep being updated over time.

Contributor

Yida-Lin commented Dec 30, 2016

@cbrnr Great idea! I am currently working on the documentation of libxdf using doxygen. Once I finish I will work on the generic subtractMean().

image

Update 12/30:

The documentation of libxdf has been uploaded to the shared google drive folder and will keep being updated over time.

@Yida-Lin

This comment has been minimized.

Show comment
Hide comment
@Yida-Lin

Yida-Lin Jan 1, 2017

Contributor

@cbrnr Guten Rutsch ins neue Jahr!!

I added the the calculating process for the mean when initializing minmax and store them in a vector as shown in the latest commit. But I need some more guidance on what to do the next; when I was doing it for XDF, I could easily subtract the mean row by row in the Xdf object. But if we want to toggle it as you suggested, (let's say click a button to toggle offsets on/off), where should I code the action? More specifically, if a file is already displayed and we click on the toggle button, the whole thing needs to be drawn again, right?

My last commit was still on the loadxdf branch for convenience reason, because if I switch between master and loadxdf back and forth, the building process is incredibly long. But if we figure out how to code the action, I will create a new branch and a separate PR.

Update 1/3/2017

I created 2 new docs in our shared google folder: the black box testing form and the draft for the paper.

Contributor

Yida-Lin commented Jan 1, 2017

@cbrnr Guten Rutsch ins neue Jahr!!

I added the the calculating process for the mean when initializing minmax and store them in a vector as shown in the latest commit. But I need some more guidance on what to do the next; when I was doing it for XDF, I could easily subtract the mean row by row in the Xdf object. But if we want to toggle it as you suggested, (let's say click a button to toggle offsets on/off), where should I code the action? More specifically, if a file is already displayed and we click on the toggle button, the whole thing needs to be drawn again, right?

My last commit was still on the loadxdf branch for convenience reason, because if I switch between master and loadxdf back and forth, the building process is incredibly long. But if we figure out how to code the action, I will create a new branch and a separate PR.

Update 1/3/2017

I created 2 new docs in our shared google folder: the black box testing form and the draft for the paper.

@cbrnr

This comment has been minimized.

Show comment
Hide comment
@cbrnr

cbrnr Jan 9, 2017

Owner

A happy new year to you too!

Thanks for your work, I'll go over it in the next days (as always, lots of stuff to do on the first day after vacation). In the meantime, you could rebase to the current master - I've integrated a fix that makes SigViewer work with Visual Studio vc++. This is nice, but since libbiosig was compiled with MinGW, it is unlikely that SigViewer compiled with vc++ can link to libbiosig. But who knows, I haven't tested it, maybe you want to give it a try? There are certainly many reports describing the issue and potential solutions.

Owner

cbrnr commented Jan 9, 2017

A happy new year to you too!

Thanks for your work, I'll go over it in the next days (as always, lots of stuff to do on the first day after vacation). In the meantime, you could rebase to the current master - I've integrated a fix that makes SigViewer work with Visual Studio vc++. This is nice, but since libbiosig was compiled with MinGW, it is unlikely that SigViewer compiled with vc++ can link to libbiosig. But who knows, I haven't tested it, maybe you want to give it a try? There are certainly many reports describing the issue and potential solutions.

@Yida-Lin

This comment has been minimized.

Show comment
Hide comment
@Yida-Lin

Yida-Lin Jan 11, 2017

Contributor

@cbrnr Hi Clemens. I tried VS but it doesn't work:

image

Also, in the email I said multi sample rate seems to work, but after a closer look, I think it's not working as expected. Though it didn't crash, but basic header need a unified sample rate to be instantiated, and despite I entered different sample rates while loading each channel, all of them are still being displayed as that unified sample rate. If the unified sample rate is 2048Hz, then the 60Hz Channel will become really short.

image

Contributor

Yida-Lin commented Jan 11, 2017

@cbrnr Hi Clemens. I tried VS but it doesn't work:

image

Also, in the email I said multi sample rate seems to work, but after a closer look, I think it's not working as expected. Though it didn't crash, but basic header need a unified sample rate to be instantiated, and despite I entered different sample rates while loading each channel, all of them are still being displayed as that unified sample rate. If the unified sample rate is 2048Hz, then the 60Hz Channel will become really short.

image

@Yida-Lin

This comment has been minimized.

Show comment
Hide comment
@Yida-Lin

Yida-Lin Jan 14, 2017

Contributor

Added a CalcEffectiveSrate() function.

image
2017-01-14 1 _li

Contributor

Yida-Lin commented Jan 14, 2017

Added a CalcEffectiveSrate() function.

image
2017-01-14 1 _li

@Yida-Lin

This comment has been minimized.

Show comment
Hide comment
@Yida-Lin

Yida-Lin Jan 15, 2017

Contributor

Bug Report

@cbrnr Josef and I found a bug: when we are in New Event mode and try to click on any channel, it crashes. I tried debugging the code, it throws a segmentation fault because the newly created event ID is -1 which caused undefined behavior. The default id_ variable in the SignalEvent class constructor is -1. But I am not quite sure how we want to fix this yet.

This happens in both XDF files and other files, like GDF. Fixed.

The nomenclature on the mean value in Baseline Adjusting function

Looks like we have both properties named "offset", but they have different meanings, which might cause confusions. Is there a better way to name the value in the baseline removal function subtractMean()?

2017-01-15_li

I temporarily renamed it to baseline. If you have a better name please let me know.

image

1/23/2017 Update:

Sigviewer can now let user add events when viewing XDF files and append those events back to the original XDF file as a new stream.

1/24/2017 Update:

Qt 5.8 has just been released and I have already successfully built Sigviewer with Qt 5.8.

1/25/2016 Update:

User can customize event text when adding new events.

2017-01-24

2017-01-24 1 _li

1/27/2016 Update:

Now each stream is automatically displayed with different color.

2017-01-27

I created an offline-installer for SigViewer using Qt Installer Framework.

image

Updated color options and removed X Grid line.

and several other updates and bug fixes.

image

Contributor

Yida-Lin commented Jan 15, 2017

Bug Report

@cbrnr Josef and I found a bug: when we are in New Event mode and try to click on any channel, it crashes. I tried debugging the code, it throws a segmentation fault because the newly created event ID is -1 which caused undefined behavior. The default id_ variable in the SignalEvent class constructor is -1. But I am not quite sure how we want to fix this yet.

This happens in both XDF files and other files, like GDF. Fixed.

The nomenclature on the mean value in Baseline Adjusting function

Looks like we have both properties named "offset", but they have different meanings, which might cause confusions. Is there a better way to name the value in the baseline removal function subtractMean()?

2017-01-15_li

I temporarily renamed it to baseline. If you have a better name please let me know.

image

1/23/2017 Update:

Sigviewer can now let user add events when viewing XDF files and append those events back to the original XDF file as a new stream.

1/24/2017 Update:

Qt 5.8 has just been released and I have already successfully built Sigviewer with Qt 5.8.

1/25/2016 Update:

User can customize event text when adding new events.

2017-01-24

2017-01-24 1 _li

1/27/2016 Update:

Now each stream is automatically displayed with different color.

2017-01-27

I created an offline-installer for SigViewer using Qt Installer Framework.

image

Updated color options and removed X Grid line.

and several other updates and bug fixes.

image

@cbrnr

This comment has been minimized.

Show comment
Hide comment
@cbrnr

cbrnr Jan 31, 2017

Owner

@Yida-Lin you integrated libxdf into SigViewer for making debugging easier - but this is not a good idea because it doesn't compile on my Mac now. The reason is that many smarc-related files #include <malloc.h>, which is non-standard and should be removed. I changed this a while ago, but my changes seem to have gotten lost somewhere.

Owner

cbrnr commented Jan 31, 2017

@Yida-Lin you integrated libxdf into SigViewer for making debugging easier - but this is not a good idea because it doesn't compile on my Mac now. The reason is that many smarc-related files #include <malloc.h>, which is non-standard and should be removed. I changed this a while ago, but my changes seem to have gotten lost somewhere.

@cbrnr

This comment has been minimized.

Show comment
Hide comment
@cbrnr

cbrnr Jan 31, 2017

Owner

Also, I get two more errors during compilation. Please fix the following issues:

  • In src/gui_impl/gui_helper_functions.cpp, change line 10 to: #include "application_context_impl.h"
  • In src/gui_impl/signal_browser/event_editing_widget.h, remove line 13 (#include "gui_impl\signal_browser\event_creation_widget.h")

Of course provided that these changes don't break your build.

Owner

cbrnr commented Jan 31, 2017

Also, I get two more errors during compilation. Please fix the following issues:

  • In src/gui_impl/gui_helper_functions.cpp, change line 10 to: #include "application_context_impl.h"
  • In src/gui_impl/signal_browser/event_editing_widget.h, remove line 13 (#include "gui_impl\signal_browser\event_creation_widget.h")

Of course provided that these changes don't break your build.

@cbrnr

This comment has been minimized.

Show comment
Hide comment
@cbrnr

cbrnr Jan 31, 2017

Owner

Also, for some reason SigViewer doesn't remember its window size any more. It always starts with maximum size. This used to work before.

Owner

cbrnr commented Jan 31, 2017

Also, for some reason SigViewer doesn't remember its window size any more. It always starts with maximum size. This used to work before.

@cbrnr

This comment has been minimized.

Show comment
Hide comment
@cbrnr

cbrnr Jan 31, 2017

Owner

Another comment: when opening an XDF file, the resampling dialog is always displayed. This is fine in general, but can be confusing when there is really only one stream in the file (maybe together with some irregular streams). This is the case in our example file Run_A_RWEO_old01.xdf. This file contains one EEG stream with 5000Hz and two streams with 0Hz (which are converted to events). In such a situation I would still offer the option to resample, but slightly rephrase the text. Maybe it would also make sense to gray out 0Hz streams.

And: I think it doesn't make sense to show the effective sample rate for irregular streams (0Hz) in the basic info dialog.

Owner

cbrnr commented Jan 31, 2017

Another comment: when opening an XDF file, the resampling dialog is always displayed. This is fine in general, but can be confusing when there is really only one stream in the file (maybe together with some irregular streams). This is the case in our example file Run_A_RWEO_old01.xdf. This file contains one EEG stream with 5000Hz and two streams with 0Hz (which are converted to events). In such a situation I would still offer the option to resample, but slightly rephrase the text. Maybe it would also make sense to gray out 0Hz streams.

And: I think it doesn't make sense to show the effective sample rate for irregular streams (0Hz) in the basic info dialog.

@Yida-Lin

This comment has been minimized.

Show comment
Hide comment
@Yida-Lin

Yida-Lin Feb 1, 2017

Contributor

Hi Clemens @cbrnr ,

  1. I have commented out all malloc.h
  2. I will try to pull libxdf out of SigViewer as soon as I finish debugging
  3. Modified those 2 lines you pointed out
  4. Now SigViewer does remember geometry and window state
  5. I agree that if there is only one sample rate the resampling dialog is redundant. After some consideration, I hid the resampling dialog if the file consists of only 1 sample rate. I thought it will expedite the loading process. If really needed, maybe we can add a new button and make the resampling function available even after a file is displayed.
  6. Now effective sample rate is no longer calculated if nominal sample rate is 0.
  7. Since there is no interaction with the user on the stream info, I would argue there is no need to grey 0Hz out. Or I can manually replace all 0Hz with more descriptive "Irregular Sample Rate" in the window for easier comprehension.

I might also need help on the file state. Currently if I add a new event or edit an event type, then save the change, it works fine. But if I edit the beginning time of event as well, even after saving it still shows file_state_changed. I will look closer into this.

Contributor

Yida-Lin commented Feb 1, 2017

Hi Clemens @cbrnr ,

  1. I have commented out all malloc.h
  2. I will try to pull libxdf out of SigViewer as soon as I finish debugging
  3. Modified those 2 lines you pointed out
  4. Now SigViewer does remember geometry and window state
  5. I agree that if there is only one sample rate the resampling dialog is redundant. After some consideration, I hid the resampling dialog if the file consists of only 1 sample rate. I thought it will expedite the loading process. If really needed, maybe we can add a new button and make the resampling function available even after a file is displayed.
  6. Now effective sample rate is no longer calculated if nominal sample rate is 0.
  7. Since there is no interaction with the user on the stream info, I would argue there is no need to grey 0Hz out. Or I can manually replace all 0Hz with more descriptive "Irregular Sample Rate" in the window for easier comprehension.

I might also need help on the file state. Currently if I add a new event or edit an event type, then save the change, it works fine. But if I edit the beginning time of event as well, even after saving it still shows file_state_changed. I will look closer into this.

@Yida-Lin

This comment has been minimized.

Show comment
Hide comment
@Yida-Lin

Yida-Lin Feb 2, 2017

Contributor

Update: this bug exists in the master branch (non-XDF) as well

2017-02-01
2017-02-01 1

I noticed that even after the saving function has set the file as unchanged, there is still a slot called setAsChanged () setting the file back as changed. I haven't figured out how to fix this yet.

Contributor

Yida-Lin commented Feb 2, 2017

Update: this bug exists in the master branch (non-XDF) as well

2017-02-01
2017-02-01 1

I noticed that even after the saving function has set the file as unchanged, there is still a slot called setAsChanged () setting the file back as changed. I haven't figured out how to fix this yet.

@cbrnr

This comment has been minimized.

Show comment
Hide comment
@cbrnr

cbrnr Feb 2, 2017

Owner

What are the steps to reproduce this issue?

Owner

cbrnr commented Feb 2, 2017

What are the steps to reproduce this issue?

@Yida-Lin

This comment has been minimized.

Show comment
Hide comment
@Yida-Lin

Yida-Lin Feb 2, 2017

Contributor
  1. Randomly add a new event to a file, gdf or xdf
  2. In Edit Event mode, randomly change the value in the Begin spinbox
  3. Try save the file or save as a new file
  4. Try to open a new file
  5. It will prompt you that the old file has not been saved yet.
  6. Even if you click No, the new file is being loaded and it then crashes
Contributor

Yida-Lin commented Feb 2, 2017

  1. Randomly add a new event to a file, gdf or xdf
  2. In Edit Event mode, randomly change the value in the Begin spinbox
  3. Try save the file or save as a new file
  4. Try to open a new file
  5. It will prompt you that the old file has not been saved yet.
  6. Even if you click No, the new file is being loaded and it then crashes
@cbrnr

This comment has been minimized.

Show comment
Hide comment
@cbrnr

cbrnr Feb 2, 2017

Owner

OK, the steps you describe actually work, but this leads to a crash:

  1. Open any file
  2. Add a new event
  3. Open a new file
  4. This will immediately show the initial min-max search window, together with the prompt (do you really want to close)

It looks like the new file is loaded to soon, without waiting for the confirmation dialog to be accepted/rejected.

Owner

cbrnr commented Feb 2, 2017

OK, the steps you describe actually work, but this leads to a crash:

  1. Open any file
  2. Add a new event
  3. Open a new file
  4. This will immediately show the initial min-max search window, together with the prompt (do you really want to close)

It looks like the new file is loaded to soon, without waiting for the confirmation dialog to be accepted/rejected.

Yida-Lin added some commits Nov 12, 2016

loadXDF
loadXDF
added close action before opening new file
Added several lines of code in open_file_gui_command.cpp to close the
old file before opening a new file, thus releasing the memory
Move the close action to later
Moved the close action to right before line 128 according to Clemens'
intruction. Close file after a valid file was successfully selected.
Sigviewer is now compatible with Qt 5.3
This repository is supposed to work with both Qt 4.8.7 and Qt 5.3
without alteration. Qt versions later than 5.3 are paired up with later
version of MinGW, which I tried but doesn't work, unless we recompile
libbiosig as well. Thus currently Qt 5.3 is the latest Qt version I can
push to. The use of conditional macro makes it backwardly compatible
with Qt 4.8.7 as well.

I found several dynamically allocated arrays was deleted using "delete"
instead of "delete[]". Are these mistakes, or supposed to work like
this?

Do you have suggestions on what else I should improve/work on? Thank
you!
loadXDF
loadXDF only, no Qt 5 compatibility
Change loadXDF return type to void
change to type void plus code clean up
Trivial code cleanup
Deleted some unnecessary comments
Added GPL license to several more files
The libxdf is temporary here: it will be removed soon
@cbrnr

This comment has been minimized.

Show comment
Hide comment
@cbrnr

cbrnr Feb 14, 2017

Owner

@Yida-Lin are you ready to remove libxdf again so that we can merge this PR? Is there anything else left to do in this PR?

Owner

cbrnr commented Feb 14, 2017

@Yida-Lin are you ready to remove libxdf again so that we can merge this PR? Is there anything else left to do in this PR?

@Yida-Lin

This comment has been minimized.

Show comment
Hide comment
@Yida-Lin

Yida-Lin Feb 14, 2017

Contributor

@cbrnr I guess pretty much that's it. The CSV support tailored for XDF can only happen after the merge. I will remove libxdf now. It might take several minutes.

Contributor

Yida-Lin commented Feb 14, 2017

@cbrnr I guess pretty much that's it. The CSV support tailored for XDF can only happen after the merge. I will remove libxdf now. It might take several minutes.

@Yida-Lin

This comment has been minimized.

Show comment
Hide comment
@Yida-Lin

Yida-Lin Feb 14, 2017

Contributor

@cbrnr Hi Clemens, I am ready. Just removed libxdf

Contributor

Yida-Lin commented Feb 14, 2017

@cbrnr Hi Clemens, I am ready. Just removed libxdf

@cbrnr

This comment has been minimized.

Show comment
Hide comment
@cbrnr

cbrnr Feb 14, 2017

Owner

Thanks. Let me do some quick final tests before I merge.

Owner

cbrnr commented Feb 14, 2017

Thanks. Let me do some quick final tests before I merge.

@Yida-Lin

This comment has been minimized.

Show comment
Hide comment
@Yida-Lin

Yida-Lin Feb 14, 2017

Contributor

@cbrnr Also, Josef earlier commented that the channel metadata on the right side is a bit cluttered (for XDF files only). However, we haven't agreed on which info to preserve and which to remove. If you have suggestions please let us know.

Contributor

Yida-Lin commented Feb 14, 2017

@cbrnr Also, Josef earlier commented that the channel metadata on the right side is a bit cluttered (for XDF files only). However, we haven't agreed on which info to preserve and which to remove. If you have suggestions please let us know.

@cbrnr

This comment has been minimized.

Show comment
Hide comment
@cbrnr

cbrnr Feb 14, 2017

Owner

Yes, I wouldn't show the baseline value, because I'd implement baseline correction differently: instead of actually subtracting the channel means, I'd adjust the y-axis locations per channel so that they are centered around the channel means.

Owner

cbrnr commented Feb 14, 2017

Yes, I wouldn't show the baseline value, because I'd implement baseline correction differently: instead of actually subtracting the channel means, I'd adjust the y-axis locations per channel so that they are centered around the channel means.

@Yida-Lin

This comment has been minimized.

Show comment
Hide comment
@Yida-Lin

Yida-Lin Feb 14, 2017

Contributor

@cbrnr That's a perfect solution. However, I need a bit of time to think how to implement that. Maybe after the merge.

Contributor

Yida-Lin commented Feb 14, 2017

@cbrnr That's a perfect solution. However, I need a bit of time to think how to implement that. Maybe after the merge.

@Yida-Lin

This comment has been minimized.

Show comment
Hide comment
@Yida-Lin

Yida-Lin Feb 14, 2017

Contributor

@cbrnr I think we only need to change several numbers on the left side (the little widget). However, currently I don't know which file contains that widget... I am looking for it

Update: seems in y_axis_widget_4.cpp

Update: This is now done in another PR.

Update: Solved some conflicts between loadXDF and master. This was done directly on GitHub that's why it's showing as a Merge commit.

Contributor

Yida-Lin commented Feb 14, 2017

@cbrnr I think we only need to change several numbers on the left side (the little widget). However, currently I don't know which file contains that widget... I am looking for it

Update: seems in y_axis_widget_4.cpp

Update: This is now done in another PR.

Update: Solved some conflicts between loadXDF and master. This was done directly on GitHub that's why it's showing as a Merge commit.

@Yida-Lin

This comment has been minimized.

Show comment
Hide comment
@Yida-Lin

Yida-Lin Feb 15, 2017

Contributor

@cbrnr Also, I remember I need to tailor the CSV function specifically for XDF a bit for some reason, I can't remember exactly why now.

Contributor

Yida-Lin commented Feb 15, 2017

@cbrnr Also, I remember I need to tailor the CSV function specifically for XDF a bit for some reason, I can't remember exactly why now.

@cbrnr

This comment has been minimized.

Show comment
Hide comment
@cbrnr

cbrnr Feb 15, 2017

Owner

Just to remind ourselves here, in our previous discussion at #23 we agreed that XDF streams should be initialized with NaNs instead of zeros. Provided that #3 is fixed (meaning that SigViewer should not draw NaNs), this would make our offset removal feature (zero line fitted) work.

Owner

cbrnr commented Feb 15, 2017

Just to remind ourselves here, in our previous discussion at #23 we agreed that XDF streams should be initialized with NaNs instead of zeros. Provided that #3 is fixed (meaning that SigViewer should not draw NaNs), this would make our offset removal feature (zero line fitted) work.

@cbrnr

This comment has been minimized.

Show comment
Hide comment
@cbrnr

cbrnr Feb 15, 2017

Owner

OK, I guess we need to take care of some things before merging. I'll put them in the top of this PR as a to do list. Please add your own points in case I forget anything.

Owner

cbrnr commented Feb 15, 2017

OK, I guess we need to take care of some things before merging. I'll put them in the top of this PR as a to do list. Please add your own points in case I forget anything.

@Yida-Lin

This comment has been minimized.

Show comment
Hide comment
@Yida-Lin

Yida-Lin Feb 15, 2017

Contributor

@cbrnr Unfortunately the CSV function doesn't exist in XDF branch so it can't be done before merging. The other two can

Contributor

Yida-Lin commented Feb 15, 2017

@cbrnr Unfortunately the CSV function doesn't exist in XDF branch so it can't be done before merging. The other two can

@cbrnr

This comment has been minimized.

Show comment
Hide comment
@cbrnr

cbrnr Feb 15, 2017

Owner

Ah OK - then let's create a separate issue so that we don't forget about that.

Owner

cbrnr commented Feb 15, 2017

Ah OK - then let's create a separate issue so that we don't forget about that.

@Yida-Lin

This comment has been minimized.

Show comment
Hide comment
@Yida-Lin

Yida-Lin Feb 16, 2017

Contributor

@cbrnr Hi Clemens,

NAN is implemented, along with several other changes that made it happen.

image

Contributor

Yida-Lin commented Feb 16, 2017

@cbrnr Hi Clemens,

NAN is implemented, along with several other changes that made it happen.

image

Set time series to NAN as default
1. Revamped getMin and getMax to take NAN into account
2. No longer call XDF detrend
3. Added condition flow not to draw line if NAN in
signal_graphics_item.cpp
4. Improved the algorithm in irregular sample rate signals
in xdfreader.cpp
@cbrnr

This comment has been minimized.

Show comment
Hide comment
@cbrnr

cbrnr Feb 16, 2017

Owner

Using NaNs was really important - thanks @Yida-Lin!

I'm going through the changes one last time. Could you please remove all icons from the basic header info dialog window? I know that it will be less clear then, but I would like to add more suitable icons at a later point.

Owner

cbrnr commented Feb 16, 2017

Using NaNs was really important - thanks @Yida-Lin!

I'm going through the changes one last time. Could you please remove all icons from the basic header info dialog window? I know that it will be less clear then, but I would like to add more suitable icons at a later point.

@Yida-Lin

This comment has been minimized.

Show comment
Hide comment
@Yida-Lin

Yida-Lin Feb 16, 2017

Contributor

@cbrnr absolutely, this is now done.

Contributor

Yida-Lin commented Feb 16, 2017

@cbrnr absolutely, this is now done.

@cbrnr

This comment has been minimized.

Show comment
Hide comment
@cbrnr

cbrnr Feb 16, 2017

Owner

Maybe for a new PR after the merge: since XDF files are time-based and not sample-based, there should be an option to export events as time in seconds vs. sample number. What do you think?

Owner

cbrnr commented Feb 16, 2017

Maybe for a new PR after the merge: since XDF files are time-based and not sample-based, there should be an option to export events as time in seconds vs. sample number. What do you think?

@Yida-Lin

This comment has been minimized.

Show comment
Hide comment
@Yida-Lin

Yida-Lin Feb 16, 2017

Contributor

@cbrnr Definitely, we can easily do this through a message box or something

Contributor

Yida-Lin commented Feb 16, 2017

@cbrnr Definitely, we can easily do this through a message box or something

@cbrnr cbrnr merged commit 69f81e5 into cbrnr:master Feb 16, 2017

@cbrnr

This comment has been minimized.

Show comment
Hide comment
@cbrnr

cbrnr Feb 16, 2017

Owner

Hooray! Thanks @Yida-Lin for your work on this great new feature!

Owner

cbrnr commented Feb 16, 2017

Hooray! Thanks @Yida-Lin for your work on this great new feature!

@Yida-Lin

This comment has been minimized.

Show comment
Hide comment
@Yida-Lin

Yida-Lin Feb 16, 2017

Contributor

@cbrnr Yay!!! :) Can't get more excited!!! Thank you for all your guidance!!

Contributor

Yida-Lin commented Feb 16, 2017

@cbrnr Yay!!! :) Can't get more excited!!! Thank you for all your guidance!!

@Yida-Lin

This comment has been minimized.

Show comment
Hide comment
@Yida-Lin

Yida-Lin Feb 16, 2017

Contributor

@cbrnr I added one more commit. I think the event type selection dialog is way too small on modern high resolution screens to my taste. I enlarged it a bit.

Contributor

Yida-Lin commented Feb 16, 2017

@cbrnr I added one more commit. I think the event type selection dialog is way too small on modern high resolution screens to my taste. I enlarged it a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment