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

Unifying Pre- and Postprocessing #61

Merged
merged 8 commits into from
Dec 18, 2020

Conversation

FynnBe
Copy link
Member

@FynnBe FynnBe commented Dec 18, 2020

No description provided.

Copy link
Collaborator

@constantinpape constantinpape left a comment

Choose a reason for hiding this comment

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

The problem is that the reference_tensor in scale_range and scale_mean_variance only makes sense in post-processing. So it is a bit awkward to unify pre-and-postprocessing and then have special cases for it in each function.

supported_formats_and_operations.md Outdated Show resolved Hide resolved
supported_formats_and_operations.md Outdated Show resolved Hide resolved
@constantinpape
Copy link
Collaborator

@FynnBe let me know when you have made all the changes and I should have a look again.

@FynnBe
Copy link
Member Author

FynnBe commented Dec 18, 2020

@FynnBe let me know when you have made all the changes and I should have a look again.

I think it's all up to date. I left binarize and sigmoid in the common processing's, as they might not be as useful for the input, but wouldn't have any other kwargs...

Copy link
Collaborator

@constantinpape constantinpape left a comment

Choose a reason for hiding this comment

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

I think you forgot scale_mean_variance in preprocessing.

supported_formats_and_operations.md Outdated Show resolved Hide resolved
supported_formats_and_operations.md Show resolved Hide resolved
supported_formats_and_operations.md Show resolved Hide resolved
- `reference_implementation`
- `scale_min_max` scale the tensor s.t. its min and max match a reference tensor
- `scale_mean_variance` scale the tensor s.t. its mean and variance match a reference tensor
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also refere to the preprocessing here.

Copy link
Member Author

Choose a reason for hiding this comment

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

should scale_mean_variance exist for preprocessing? It is different from the zero_mean_unit_variance.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think for scale_mean_variance we only need to know the reference tensor, correct?
Then it is all correct like it is right now, because we actually don't need to know any other kwargs here.

supported_formats_and_operations.md Outdated Show resolved Hide resolved
@FynnBe
Copy link
Member Author

FynnBe commented Dec 18, 2020

I think you forgot scale_mean_variance in preprocessing.

zero_mean_unit_variance is in the Pre- and Postprocessing section...

@constantinpape
Copy link
Collaborator

zero_mean_unit_variance is in the Pre- and Postprocessing section...

But then same as preprocessing in the kwargs is ambiguous and we need to refer to zero_mean_unit_variance there.

@FynnBe
Copy link
Member Author

FynnBe commented Dec 18, 2020

But then same as preprocessing in the kwargs is ambiguous and we need to refer to zero_mean_unit_variance there.

same as preprocessing is now only specified for scale_range

@FynnBe
Copy link
Member Author

FynnBe commented Dec 18, 2020

actually for scale_mean_variance a mode=fixed would also makes sense, but one can achieve the same with zero_mean_unit_variance + scale_linear. Therefore I would leave it at that for now.

Copy link
Collaborator

@constantinpape constantinpape left a comment

Choose a reason for hiding this comment

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

Ok, I think I understand it know and it should all be correct:
For scale_mean_variance we only need to know the reference_tensor as kwarg, so we actually don't need to refer to any other kwargs.

- `reference_implementation`
- `scale_min_max` scale the tensor s.t. its min and max match a reference tensor
- `scale_mean_variance` scale the tensor s.t. its mean and variance match a reference tensor
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think for scale_mean_variance we only need to know the reference tensor, correct?
Then it is all correct like it is right now, because we actually don't need to know any other kwargs here.

@FynnBe
Copy link
Member Author

FynnBe commented Dec 18, 2020

Ok, I think I understand it know and it should all be correct:
For scale_mean_variance we only need to know the reference_tensor as kwarg, so we actually don't need to refer to any other kwargs.

The mode (per_sample or per_dataset) is required as well to determine how to determine mean and variance from the reference.

@constantinpape
Copy link
Collaborator

The mode (per_sample or per_dataset) is required as well to determine how to determine mean and variance from the reference.

True, but we have that already.
I just wanted to point out that we don't need to refer to any other kwargs.

This is good to go now and I will merge.

@constantinpape constantinpape merged commit 2c4f7df into shape-description-and-authors Dec 18, 2020
@constantinpape constantinpape deleted the processing branch December 18, 2020 15:07
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.

None yet

2 participants