Add scaling_range and new test for scaling_range #57
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tilman151
requested changes
Mar 21, 2024
| ) -> None: | ||
| """ | ||
| Create a new reader for the New C-MAPSS dataset. The maximum RUL value is set | ||
| to 65 by default. The default channels are the four operating conditions, | ||
| to 65 by default. The default channels are the four operating conditions, |
There was a problem hiding this comment.
There is one white space missing.
| features, _, _ = self._load_data("dev") | ||
| scaler = scaling.fit_scaler(features, MinMaxScaler()) | ||
| scaling.save_scaler(scaler, self._get_scaler_path()) | ||
| #if not os.path.exists(self._get_scaler_path()): |
There was a problem hiding this comment.
We will obviously need to find a solution for this. The scaling range should be part of the scaler_path. Try to modify _get_scaler_path to take the range into account. This way it would refit the scaler if the range changed.
| reader.prepare_data() | ||
| features_default, _ = reader.load_split("dev") | ||
|
|
||
| assert not np.array_equal(features[0][:, :, 1], features_default[0][:, :, 1]) |
There was a problem hiding this comment.
I think testing min and max of the features would be a more suitable test. After scaling features should be inside the scaling range. Additionally, you should test all elements of features, not only the first one. It is okay to have an assert inside a for loop.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Hallo.
Ich die Pull Request mit Git Bash gemacht, hoffe es in Ordnung ist.
Außer was ich beim letzten Mal geändert habe, wird es eine neue Test addiert, um den "scaling_range" zu testen. Dabei habe ich aber die
"if not os.path.exists(self._get_scaler_path()):" in ncmapss.py ignoriert, da solang es ein scaling file schon hat, wird kein neu "scaling_range" akzeptiert.
Ich versuch weitere Löusung zu finden, aber ich denke mal, dass ich was ich bis jetzt habe schon mal schicken