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

Error Handling #429

Merged
merged 6 commits into from
Mar 18, 2022
Merged

Error Handling #429

merged 6 commits into from
Mar 18, 2022

Conversation

fealho
Copy link
Member

@fealho fealho commented Mar 9, 2022

Resolve #408.

@codecov-commenter
Copy link

codecov-commenter commented Mar 9, 2022

Codecov Report

Merging #429 (991502a) into v1.0.0.dev (01ed965) will not change coverage.
The diff coverage is 100.00%.

@@              Coverage Diff              @@
##           v1.0.0.dev      #429    +/-   ##
=============================================
  Coverage      100.00%   100.00%            
=============================================
  Files              14        14            
  Lines            1059      1229   +170     
=============================================
+ Hits             1059      1229   +170     
Impacted Files Coverage Δ
rdt/hyper_transformer.py 100.00% <100.00%> (ø)
rdt/errors.py 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01ed965...991502a. Read the comment docs.

@fealho fealho force-pushed the issue-408-error-handling branch from 6172d38 to dd71395 Compare March 15, 2022 16:35
Copy link
Contributor

@amontanez24 amontanez24 left a comment

Choose a reason for hiding this comment

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

This is close! Some minor changes

"""
if not self._config_detected:
warnings.warn(
"Tip: You can use the method 'detect_initial_config' to inspect "
Copy link
Contributor

Choose a reason for hiding this comment

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

Although the issue specifies to raise this if the detect method wasn't called, I think this warning should show whenever a config isn't present. Instead of checking if the method was called just check if there are any values set in the field_transformers and field_sdtypes

Comment on lines 490 to 493
'The data you are trying to fit has different columns than the original '
f'detected data (unknown columns: {unknown_columns}). Column names and their '
"sdtypes must be the same. Use the method 'get_config()' to see the expected "
'values.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this message to a variable since it is duplicated?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not actually a duplicate, they are just very similar.

the tip will no longer be shown (for an instance of the HyperTransformer).
"""
if not self._config_detected:
warnings.warn(
Copy link
Contributor

Choose a reason for hiding this comment

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

The issue was changed to make this an error

@fealho fealho marked this pull request as ready for review March 17, 2022 17:01
@fealho fealho requested a review from a team as a code owner March 17, 2022 17:01
@fealho fealho requested review from pvk-developer and amontanez24 and removed request for a team and pvk-developer March 17, 2022 17:01
Copy link
Member

@pvk-developer pvk-developer left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@amontanez24 amontanez24 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 this is good to go!

@pvk-developer pvk-developer merged commit 99fe422 into v1.0.0.dev Mar 18, 2022
@pvk-developer pvk-developer deleted the issue-408-error-handling branch March 18, 2022 20:02
amontanez24 pushed a commit that referenced this pull request Apr 21, 2022
* Add tip for detect_initial_config

* Add the rest of the checks

* Update errors + add tests + remove sdtypes check

* Address feedback

* Fix documentation

* Fix lint

Co-authored-by: Plamen Valentinov Kolev <[email protected]>
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.

4 participants