Dependency conflict fix by djmarques · Pull Request #160 · ghammad/pyActigraphy

djmarques · 2024-12-12T17:14:04Z

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.

This reverts commit ec80e6c.

…age accelerometer. Here, I natively implement date_parser and imputeMissing.

ghammad

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.

ghammad · 2024-12-13T10:15:28Z

README.rst



-**pyActigraphy**
+**pyActigraphy** 


Is it needed? I do not see any formatting issue on GitHub.com, nor on GitHub.io.

This is not required. I was just experimenting GitHub desktop functionality. The extra space has been removed.

ghammad · 2024-12-13T10:16:24Z

pyActigraphy/io/bba/bba.py

-from accelerometer.utils import date_parser
-from accelerometer.summarisation import imputeMissing
+from pandas.tseries.frequencies import to_offset
+#from accelerometer.utils import date_parser


Please remove old imports/commented lines.

ghammad · 2024-12-13T10:29:49Z

pyActigraphy/io/bba/bba.py

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


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

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

ghammad · 2024-12-13T10:30:12Z

pyActigraphy/io/bba/bba.py

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


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)

ghammad · 2024-12-13T10:36:50Z

pyActigraphy/io/bba/bba.py

+    t = re.sub(r'\[(.*?)\]', '', t)
+    return pd.to_datetime(t, utc=True).tz_convert(tz)
+
+def imputeMissing(data, extrapolate=True):


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

I've now included the function fillna.

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

I think the function is now completely added! Also, imported numpy as it is required for fillna to work. Thanks for your assistance!

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.

FWIW, the accelerometer license is strictly speaking not open source, and including it here conflicts with the GPL license of pyActigraphy.

I wonder if a better solution would be to use importlib to dynamically import accelerometer when needed in bba.py, and to fail with an error message containing installation instructions if the package is not already available. That way you wouldn't need to copy over the code, but could still remove the accelerometer dependency for anyone who doesn't need support for this file type.

See #161 for the suggested alternative.

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

ghammad

Almost there. Thanks for your work.

ghammad · 2024-12-13T13:34:36Z

pyActigraphy/io/bba/bba.py

+    t = re.sub(r'\[(.*?)\]', '', t)
+    return pd.to_datetime(t, utc=True).tz_convert(tz)
+
+def imputeMissing(data, extrapolate=True):


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

djmarques added 4 commits December 12, 2024 16:08

Update README.rst

7faaec8

Update README.rst

ec80e6c

Revert "Update README.rst"

3dc3e50

This reverts commit ec80e6c.

Correction of some dependency conflicts regarding the use of the pack…

92d2a00

…age accelerometer. Here, I natively implement date_parser and imputeMissing.

ghammad self-assigned this Dec 13, 2024

ghammad requested changes Dec 13, 2024

View reviewed changes

djmarques added 4 commits December 13, 2024 11:53

Update bba.py

de173ae

Removed old imports/commented lines

Removed space after "**pyActigraphy**"

745c74b

Correction of some dependency conflicts regarding the use of the pack…

51a83ba

…age accelerometer. Here, I natively implement date_parser and imputeMissing. I added a docstring and fillna in imputeMissing.

Merge remote-tracking branch 'origin/master'

3389801

# Conflicts: # pyActigraphy/io/bba/bba.py

ghammad requested changes Dec 13, 2024

View reviewed changes

Completing imputeMissing

88b76c6

jukka mentioned this pull request Jan 7, 2025

Make the accelerometer dependency optional #161

Open

djmarques closed this Jan 7, 2025



		pyActigraphy
		pyActigraphy

Conversation

djmarques commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghammad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djmarques Dec 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djmarques Dec 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghammad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

djmarques commented Dec 12, 2024 •

edited

Loading

djmarques Dec 13, 2024 •

edited

Loading

djmarques Dec 13, 2024 •

edited

Loading