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

Implementation of OPCR1 #82

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

FlorentinBulotAQ
Copy link
Contributor

Hello,

I have written an implementation for the OPC-R1. I have tested methods: on(), off(), histogram() and reset(). I have not yet tested the other methods.

Would it be possible to publish it?

With best wishes,
Florentin.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.3%) to 13.676% when pulling edd78ad on FlorentinBulotUoS:master into 2c8f195 on dhhagan:master.

@coveralls
Copy link

coveralls commented Feb 22, 2019

Coverage Status

Coverage decreased (-2.3%) to 13.676% when pulling 9dfae9d on FlorentinBulotUoS:master into 2c8f195 on dhhagan:master.

@FlorentinBulotAQ
Copy link
Contributor Author

I do not know much about code coverage. Do you have a script with unit testing that I should add methods to ?

@dhhagan
Copy link
Owner

dhhagan commented Feb 22, 2019

Hi @FlorentinBulotUoS - don't worry about the code coverage - hardware products are difficult to test in this way (at least in my experience). I'll check the code and merge the PR after my committee meeting today. Thanks!

@FlorentinBulotAQ
Copy link
Contributor Author

Hi @dhhagan,

Let me know if there is any issue with the code for me to fix.

With best wishes.

@dhhagan
Copy link
Owner

dhhagan commented Mar 3, 2019

Hey @FlorentinBulotUoS I will do my best to get to this tonight/tomorrow - currently swamped trying to finish up a manuscript...

@dhhagan
Copy link
Owner

dhhagan commented Mar 12, 2019

Hey @FlorentinBulotUoS Thanks so much for this work - I'm almost done going through it and will give any feedback if needed, otherwise I will merge it in to master. If possible, it would be ideal to have more than 1 person test it before the actual release to pip - do you know of anyone else who could test it? If not, I can try to reach out to Alphasense...

@FlorentinBulotAQ
Copy link
Contributor Author

Hi @dhhagan, thanks a lot for going through the code.
I will check with one of my colleague who may be able to test it on Friday. I have sent the code to Alphasense last week but I have not heard back from them yet (although I have not asked them explicitly if they could test it...).

Copy link
Owner

@dhhagan dhhagan left a comment

Choose a reason for hiding this comment

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

Hey @FlorentinBulotUoS Thanks again for this contribution. Before I go ahead and pull it into master, can you go ahead and change the function names and arguments to be lowercase with underscores to separate words. I can do it if you prefer - just let me know. Otherwise, I don't see any major issues, but can't test it myself. Once those changes are made, I will merge into master and upgrade the version to 1.7.0 - I think I will leave it in master until a few others can test, at which point I will push the new version to pypi. Let me know if you have any questions/concerns.

@FlorentinBulotAQ
Copy link
Contributor Author

Hi @dhhagan, sorry for the late answer. I have done the changes you have requested. The code has not been tested yet by somebody other than me.
Let me know if everything is fine with it.

Kind regards

@FlorentinBulotAQ
Copy link
Contributor Author

Hi @dhhagan,

I have fixed a couple of issues that appeared while running the code for an extended period of time on multiple OPCR1. I have also fixed the read_config() and pm() methods which were broken. I was wondering if you could merge it with the master so people can test it?

Thanks a lot,
With best wishes.

@Smeedy
Copy link

Smeedy commented Oct 24, 2019

Hi there,

I was looking for the OPC-N3 implementation. You are talking about the OPC-R1, which on the product page is mentioned in the last alinea. They switch from OPC-N3 to OPC-R1. This is the same device?

I have 6 of them here up for testing and will look at code and lab results if we are indeed talking about the same device.

cheers

@FlorentinBulotAQ
Copy link
Contributor Author

Hi @Smeedy ,

The OPC-R1 and the OPC-N3 are two different models. The OPC-N3 is more expensive and can measure particles <40um diameter while the OPC-R1 measures particles <10um.

I do not know whether they use the same commands though so I am not sure whether the implementation of the OPC-R1 will work for the OPC-N3. I do not have any OPC-N3 to test it so I cannot tell you more.

I hope that helps,
With best wishes

@FlorentinBulotAQ
Copy link
Contributor Author

Dear contributors,

Just to let you know that I needed to reference the software for the OPC-R1 so have made a release of my fork for the OPC-R1 and have obtained a DOI for it: https://zenodo.org/record/3571966

I have used the information I had on GitHub about each of you. Let me know if you want me to edit your details on the DOI (names, ORCID and affiliations).

With best wishes,
Florentin.

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.

4 participants