-
Notifications
You must be signed in to change notification settings - Fork 13
fix edexplore #276
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
base: dev
Are you sure you want to change the base?
fix edexplore #276
Conversation
theRealSuperMario
commented
Jul 18, 2020
- now accepts new dataset configuriation
- fix bug in initialization
* now accepts new dataset configuriation * fix bug in initialization
| # additional visualizations | ||
| default_additional_visualizations = retrieve( | ||
| config, "edexplore/visualizations", default=dict() | ||
| config, "edexplore/visualizations", default={} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://stackoverflow.com/a/5790954
indicates a performance improvement for this change. For consisntency however, I would not change only a single instance.
should we change dict() to {} then everywhere at leas in this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this does not matter. I was just fixing a bug when default was not given an empty dict.
| module_name = retrieve(config, "datasets/train") | ||
| Dataset = get_obj_from_str(module_name) | ||
| else: | ||
| raise KeyError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should also add the possiblity to explore the validation dataset? Maybe with a selector in the sidebar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was only fixing a bug that got introduced here. I am not willing to implement a new feature at this point.
I think it is not important enough to implement this feature at the moment because you can just hack around it.