-
Notifications
You must be signed in to change notification settings - Fork 28
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
Dependency conflict fix #160
base: master
Are you sure you want to change the base?
Conversation
This reverts commit ec80e6c.
…age accelerometer. Here, I natively implement date_parser and imputeMissing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dear @naja23
thanks a lot for the PR. Wonderfull to see user's involvement in making pyActigraphy better/easier to use.
While you address my comments, I will contact the biobankanalysis (accelerometer) developers to clarify our intentions. At the moment, their code is duplicated in your PR. And, as it has a license, we need to make sure this PR is aligned with their license requirements. However, as these are small modifications (and yet important), I do expect a positive outcome. I will get you posted.
README.rst
Outdated
@@ -14,7 +14,7 @@ | |||
:target: CODE_OF_CONDUCT.md | |||
|
|||
|
|||
**pyActigraphy** | |||
**pyActigraphy** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it needed? I do not see any formatting issue on GitHub.com, nor on GitHub.io.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not required. I was just experimenting GitHub desktop functionality. The extra space has been removed.
pyActigraphy/io/bba/bba.py
Outdated
from accelerometer.utils import date_parser | ||
from accelerometer.summarisation import imputeMissing | ||
from pandas.tseries.frequencies import to_offset | ||
#from accelerometer.utils import date_parser |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove old imports/commented lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
pyActigraphy/io/bba/bba.py
Outdated
# Native implementation of date_parser and imputeMissing | ||
def date_parser(t): | ||
""" | ||
Adapted from the accelerometer package (utils.py) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you:
- provide a link to the accelerometer package? And the version you used (for future ref)? A tag is fine (for ex: v7.2.1);
- add a description for the function as well as for its input parameters (cf other functions in this class).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've provided a link to accelerometer 7.2.1 and the copyright notice as per the license.md. I think our use is covered by paragraph 1.2, which grants licence to "reproduce, modify, transmit (...) for academic, research or other scholarly use.".
pyActigraphy/io/bba/bba.py
Outdated
|
||
def imputeMissing(data, extrapolate=True): | ||
""" | ||
Adapted from the accelerometer package (summarisation.py). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you:
- provide a link to the accelerometer package? And the version you used (for future ref)? A tag is fine (for ex: v7.2.1);
- add a description for the function as well as for its input parameters (cf other functions in this class)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
t = re.sub(r'\[(.*?)\]', '', t) | ||
return pd.to_datetime(t, utc=True).tz_convert(tz) | ||
|
||
def imputeMissing(data, extrapolate=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment, this function does not impute missing data but rather pad the data boundaries so that it covers a full day (24h). It misses the fillna function and its application (see https://github.com/OxWearables/biobankAccelerometerAnalysis/blob/ed0caad712a9686deb2bf3a37273c1dedd14beb6/src/accelerometer/summarisation.py#L250).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've now included the function fillna
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@naja23 thanks for adding fillna
. However it still misses the rest of the function from (https://github.com/OxWearables/biobankAccelerometerAnalysis/blob/ed0caad712a9686deb2bf3a37273c1dedd14beb6/src/accelerometer/summarisation.py#L264) up to return data
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the function is now completely added! Also, imported numpy as it is required for fillna to work. Thanks for your assistance!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes. As mentioned earlier, I started a discussion with the accelerometer
package developers. Let see how it goes before moving on the PR.
Removed old imports/commented lines
…age accelerometer. Here, I natively implement date_parser and imputeMissing. I added a docstring and fillna in imputeMissing.
# Conflicts: # pyActigraphy/io/bba/bba.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there. Thanks for your work.
t = re.sub(r'\[(.*?)\]', '', t) | ||
return pd.to_datetime(t, utc=True).tz_convert(tz) | ||
|
||
def imputeMissing(data, extrapolate=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@naja23 thanks for adding fillna
. However it still misses the rest of the function from (https://github.com/OxWearables/biobankAccelerometerAnalysis/blob/ed0caad712a9686deb2bf3a37273c1dedd14beb6/src/accelerometer/summarisation.py#L264) up to return data
.
Fixed the dependency conflict generated by the accelerometer package. Instead of importing the
date_parser
andimputeMissing
functions from accelerometer, I natively implemented these functions. Also removed the requirement for accelerometer instalation insetup.py
.