Skip to content

Pe/fix#36

Open
shamoons wants to merge 11 commits intomaxjcohen:masterfrom
shamoons:pe/fix
Open

Pe/fix#36
shamoons wants to merge 11 commits intomaxjcohen:masterfrom
shamoons:pe/fix

Conversation

@shamoons
Copy link
Contributor

No description provided.

shamoons and others added 4 commits December 11, 2020 12:15
Co-authored-by: Max Cohen <max.zagouri@pm.me>
* Added a period parameter to the transformer

* Fix

Co-authored-by: Max Cohen <max.zagouri@pm.me>
@shamoons
Copy link
Contributor Author

There was an error that this fixes @maxjcohen

@maxjcohen
Copy link
Owner

Hi, if you're talking about the default value of pe_period, I would like to keep it as is. It's well documented too. Do tell me if there is something else I haven't seen here.

@shamoons
Copy link
Contributor Author

I'll update it to keep the default period. The actual error is caused here: https://github.com/maxjcohen/transformer/blob/master/tst/transformer.py#L129

so I'll come up with another solution and update this PR accordingly

@maxjcohen
Copy link
Owner

I see, I'll let you work on it, and see if I can find a alternative later this week.

@shamoons
Copy link
Contributor Author

Give me a day or 2 if you can and I'll have something

@shamoons
Copy link
Contributor Author

@maxjcohen check the update. This should work now

shamoons and others added 6 commits December 18, 2020 13:18
* Added a period parameter to the transformer

* Fix

* Fixed

Co-authored-by: Max Cohen <max.zagouri@pm.me>
* Added a period parameter to the transformer

* Fix

* Fixed

* Edge check

Co-authored-by: Max Cohen <max.zagouri@pm.me>
* Added a period parameter to the transformer

* Fix

* Fixed

* Edge check

Co-authored-by: Max Cohen <max.zagouri@pm.me>
Comment on lines +50 to +52
Must be one of `original, `regular` or `None. Default is `None`.
pe_period:
If using the ``'regular'` pe, then we can define the period. Default is ``24``.
If using the ``regular`` pe, then we can define the period. Default is ``24``.
Copy link
Owner

Choose a reason for hiding this comment

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

The correct syntax here should be:

  • ``'original'`` for string values
  • ``24`` for number values
  • ``None`` for None

chunk_mode: str = 'chunk',
pe: str = None,
pe_period: int = 24):
pe_period: int = None):
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer having a default value here, of 24.

Comment on lines +98 to +100

if pe == 'regular' and pe_period is not None:
self._pe_period = pe_period
Copy link
Owner

Choose a reason for hiding this comment

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

This seems unnecessary, what edge case does it address ?

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