ScreenResolution, Start&StopTime warning #26
Merged
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
Member
julia-pfarr
commented
Sep 13, 2023
julia-pfarr
commented
- changed where to get ScreenResolution from: now getting it from "GAZE_COORDS" instead of "DISPLAY_COORDS"
- throw warning when multiple Start&StopTimes according to issue#2
- "DISPLAY_COORDS" is apparently not present in every asc file, thus changed extraction of ScreenResolution to get it from "GAZE_COORDS" - put underscore before eyetrack./events.json output file spec
for more information, see https://pre-commit.ci
Merge github.com:julia-pfarr/eye2bids
for more information, see https://pre-commit.ci
Remi-Gau
reviewed
Sep 14, 2023
Comment on lines
+224
to
+229
| return ( | ||
| (df[df[2] == "GAZE_COORDS"]) | ||
| .iloc[0:1, 3:5] | ||
| .to_string(header=False, index=False) | ||
| .replace(".00", "") | ||
| .split(" ") |
There was a problem hiding this comment.
this should return integer values and currently it is returning strings
that's why the tests are failing
Remi-Gau
reviewed
Sep 14, 2023
Comment on lines
+249
to
+251
| """Your input file contains multiple start times.\n | ||
| As this is not seen as good practice in eyetracking experiments, only the first start time will be kept for the metadata file.\n | ||
| Please consider changing your code accordingly for future eyetracking experiments.\n""" |
There was a problem hiding this comment.
Suggested change
| """Your input file contains multiple start times.\n | |
| As this is not seen as good practice in eyetracking experiments, only the first start time will be kept for the metadata file.\n | |
| Please consider changing your code accordingly for future eyetracking experiments.\n""" | |
| """Your input file contains multiple start times.\n | |
| As this is not seen as good practice in eyetracking experiments, \n | |
| only the first start time will be kept for the metadata file.\n | |
| Please consider changing your code accordingly for future eyetracking experiments.\n""" |
Remi-Gau
reviewed
Sep 14, 2023
Comment on lines
+265
to
+267
| """Your input file contains multiple stop times.\n | ||
| As this is not seen as good practice in eyetracking experiments, only the last stop time will be kept for the metadata file.\n | ||
| Please consider changing your code accordingly for future eyetracking experiments.\n""" |
There was a problem hiding this comment.
Suggested change
| """Your input file contains multiple stop times.\n | |
| As this is not seen as good practice in eyetracking experiments, only the last stop time will be kept for the metadata file.\n | |
| Please consider changing your code accordingly for future eyetracking experiments.\n""" | |
| """Your input file contains multiple stop times.\n | |
| As this is not seen as good practice in eyetracking experiments, \n | |
| only the last stop time will be kept for the metadata file.\n | |
| Please consider changing your code accordingly for future eyetracking experiments.\n""" |
Remi-Gau
reviewed
Sep 14, 2023
| .tolist() | ||
| ) | ||
| if len(StopTime) > 1: | ||
| e2b_log.info( |
There was a problem hiding this comment.
Suggested change
| e2b_log.info( | |
| e2b_log.warning( |
Remi-Gau
reviewed
Sep 14, 2023
| .tolist() | ||
| ) | ||
| if len(StartTime) > 1: | ||
| e2b_log.info( |
There was a problem hiding this comment.
Suggested change
| e2b_log.info( | |
| e2b_log.warning( |
There was a problem hiding this comment.
I think we may to just use warning.warns instead of using the logger but we can decide on that later
|
FYI: in general it is better to tackle only one "problem" per pull request: in this one we are working on both the start / stop time AND the screen resolution. One PR for each makes it easier to review |
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.