Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Refactorised tabularisation + Jupyter notebook w/ experiments. #1399
Refactorised tabularisation + Jupyter notebook w/ experiments. #1399
Changes from 19 commits
52bee74
748caf5
83d78b1
9356cd7
85011fe
79b4fe7
c64b330
ab48acf
ededa0a
9e954db
ab548c5
fd3e5ce
8be63d1
5b25175
9800cbd
6298bb1
9da4ade
4c1313f
01a6b14
479ba8d
7fa3eea
2766c68
6bda371
722765d
9baa820
cadb51c
7002447
e38e240
a4ee267
ca54b13
8dab315
7845c60
4c7c163
557d100
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
wouldn't calling _add_static_covariates() better done inside the preceding for loop? in my opinion, this would help simplify the _add_static_covariates() internal logic since, if it only takes a specific target (from the input sequence) and its specific features, than one can avoid looping twice over the series inside the _add_static_covariates(), if I understood the implementation correctly. WDYT?
In the current _add_static_covariates() my assumption was that the function will receive all the features and the function should compute back everything it needs in terms of length and width of features, since I was expecting a change in the _create_lagged_data() outputs, but this might not be potentially relevant anymore with the changes you made.
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.
It would still be necessary though to go through all the series in the input sequence once, prior, to collect the static covariates information from all of them.
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.
Definitely - in my opinion, the static covariates would ideally be added inside of
create_lagged_data
after each 'block' has been formed, but that would probably be a bit clumsy to implement at the moment since the process of computing the static covariates requires then_features_in_
attribute of theRegressionModel
object. Perhaps something to think about for a future PR?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'm still wondering whether this shouldn't belong to
tabularization.py
. It is specific toRegressionModel
s, but so are the other tabularization functions in there. And I think handling static covariates is part of the tabularization. WDYT?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.
Hmmm - definitely seeing where you're coming from. I suppose my main hesitation at the moment is that the process of computing the static covariates seems to depend a lot on the attributes of
RegressionModel
, and, from what I understand,_add_static_covariates
is only going to be used byRegressionModel
, which tends to give me the impression that_add_static_covariates
is basically acting as a method forRegressionModel
here.Is there somehow a way to 'decouple' computing the actual static covariates values from the
RegressionModel
object? If so, an alternative approach would be to attach the static covariates to each block inside ofcreate_lagged_data
as soon as each block is formed (as I noted under one of @eliane-maalouf 's suggestions).