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

Dependency conflict fix #160

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

Dependency conflict fix #160

wants to merge 9 commits into from

Conversation

djmarques
Copy link

@djmarques djmarques commented Dec 12, 2024

Fixed the dependency conflict generated by the accelerometer package. Instead of importing the date_parser and imputeMissing functions from accelerometer, I natively implemented these functions. Also removed the requirement for accelerometer instalation in setup.py.

@ghammad ghammad self-assigned this Dec 13, 2024
Copy link
Owner

@ghammad ghammad left a 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**
Copy link
Owner

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.

Copy link
Author

@djmarques djmarques Dec 13, 2024

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.

from accelerometer.utils import date_parser
from accelerometer.summarisation import imputeMissing
from pandas.tseries.frequencies import to_offset
#from accelerometer.utils import date_parser
Copy link
Owner

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.

Copy link
Author

Choose a reason for hiding this comment

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

Done!

# Native implementation of date_parser and imputeMissing
def date_parser(t):
"""
Adapted from the accelerometer package (utils.py)
Copy link
Owner

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).

Copy link
Author

@djmarques djmarques Dec 13, 2024

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.".


def imputeMissing(data, extrapolate=True):
"""
Adapted from the accelerometer package (summarisation.py).
Copy link
Owner

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)

Copy link
Author

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):
Copy link
Owner

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).

Copy link
Author

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.

Copy link
Owner

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.

Copy link
Author

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!

Copy link
Owner

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
Copy link
Owner

@ghammad ghammad left a 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):
Copy link
Owner

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.

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.

2 participants