Skip to content
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

sensor_size default to None on some transforms #277

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

HussainMSajwani
Copy link

ToFrame and DropEvent and their functional counterparts handle sensor_size being None internally. I have changed those two to accept sensor_size being None. This is useful for datasets where sensor_size is not given such as NCaltech101.

Some other transforms technically do not need sensor_size but we can handle those in a future pull request.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (debdd2e) 76.98% compared to head (67bf7cd) 77.01%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #277      +/-   ##
===========================================
+ Coverage    76.98%   77.01%   +0.03%     
===========================================
  Files           54       54              
  Lines         3059     3059              
===========================================
+ Hits          2355     2356       +1     
+ Misses         704      703       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -189,7 +189,7 @@ class DropEventByArea:
specified ratio of the sensor size.

Args:
sensor_size (Tuple): size of the sensor that was used [W,H,P]
sensor_size (Optional[Tuple[int, int, int]]): size of the sensor that was used [W,H,P]. Defaults to None.
Copy link
Member

Choose a reason for hiding this comment

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

Here we need to explain what setting it to None means, that it'll automatically calculate the sensor size from the maximum event locations in the data and that that is not a guarantee that it is the true sensor size

@biphasic
Copy link
Member

biphasic commented Jan 4, 2024

The problem with calculating the sensor size automatically every time is that users will omit the parameter because it works most of the times. But then sometimes, not all the samples in a batch will have the same calculated sensor size. During batching, this will throw an error saying that it can only stack tensors of the same size. I think I prefer that the user has to be explicit about this

@HussainMSajwani
Copy link
Author

The reason I created this pull request is that I was trying to use some of the transforms on N-Caltech10, which has variable size, and most expected a sensor_size argument. This makes the transforms not usable on N-Caltech101.

Some transforms already handle missing sensor_size internally. See for example ToFrame. But still an error is thrown when sensor_size is not passed. This pull request only allows sensor_size not to be passed.

Perhaps a warning should be displayed to the user when the sensor_size is not declared explicitly. This allows the transforms to work on N-Caltech101 and still alerts the user of potential errors downstream coming from non fixed sensor_size. I can help making this a thing if you agree.

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.

3 participants