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

feat: add channel_labels #323

Merged
merged 6 commits into from Nov 9, 2020
Merged

feat: add channel_labels #323

merged 6 commits into from Nov 9, 2020

Conversation

abawchen
Copy link
Contributor

@abawchen abawchen commented Mar 9, 2020

Add channel_labels and pass unittest.

@abawchen
Copy link
Contributor Author

abawchen commented Mar 9, 2020

@castillohair @JS3xton : Please help review this one, thanks.

Copy link
Contributor

@JS3xton JS3xton left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. I found some small things, but otherwise looks good to me.

  1. The channel_labels method should be described in the FCSData docstring.
  2. It would be nice to explicitly unit test channel_labels like other similar methods (e.g. the TestFCSAttributesDetectorVoltage class in /test/test_io.py).

@JS3xton
Copy link
Contributor

JS3xton commented May 2, 2020

  1. Add channel_labels method to TestFCSDataPickle in /test/test_io.py.

@abawchen
Copy link
Contributor Author

abawchen commented May 4, 2020

@JS3xton : Thanks for feedback, I will update accordingly.

@abawchen
Copy link
Contributor Author

abawchen commented May 5, 2020

@JS3xton : I have a quick question, because all of the current testing fcs files don't have label value, i.e. they don't have $PnS key, and will return [None, None, ..., None] of all channels -- should I modify the current testing fcs file or create another one?

@JS3xton
Copy link
Contributor

JS3xton commented May 5, 2020

@JS3xton : I have a quick question, because all of the current testing fcs files don't have label value, i.e. they don't have $PnS key, and will return [None, None, ..., None] of all channels -- should I modify the current testing fcs file or create another one?

@abawchen Hmm, good point. I wouldn't modify the existing files. If you have a new file you can contribute, that seems fine to me. Otherwise, you might be able to find one in this repository: https://flowrepository.org/id/FR-FCM-ZZZ4.

@castillohair
Copy link
Collaborator

@JS3xton , I added the description of channel_labels into the FCSData docstring. I also added it to the TestFCSDataPickle test.

@abawchen I think we would still prefer if you could incorporate an FCS file that actually contains $PnS keys and make unit tests for it. Code looks good, but we still have no "experimental" evidence that it does what it's supposed to do.

@abawchen
Copy link
Contributor Author

abawchen commented Oct 26, 2020

@JS3xton @castillohair :
Hi, thanks your feedback, although it's been a while 😅
I have added Data005.fcs and add corresponding test case as you suggested, please help check it.

btw, I have rebase the updated develop branch.


We have previously looked at the contents of the $PnS attribute for
the test files and identified the correct label(marker):
- Data001.fcs: [None, None, None, None, None, None]
Copy link
Collaborator

Choose a reason for hiding this comment

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

The last element of this list should be 'Time (204.80 sec.)'. This is what you have later in the test_attribute() function.

@castillohair
Copy link
Collaborator

In my opinion, the proposed unit tests validate that the feature works as expected. I have one observation in the test class docstring though. @JS3xton see what you think.

-Changed "PxS" to "PnS" to conform to naming convention for FCS
 parameters.
@JS3xton JS3xton self-requested a review October 29, 2020 23:14
@JS3xton
Copy link
Contributor

JS3xton commented Oct 29, 2020

Looks good to me.

(The new data file is kinda large, though... ~16MB)

@castillohair castillohair merged commit 8e92580 into taborlab:develop Nov 9, 2020
@castillohair
Copy link
Collaborator

@abawchen thanks for the contribution! We will release it with the new FlowCal version soon.

@abawchen abawchen deleted the develop branch November 13, 2020 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants