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

Fixes and refactoring #27

Merged
merged 29 commits into from
Jun 4, 2019
Merged

Fixes and refactoring #27

merged 29 commits into from
Jun 4, 2019

Conversation

kraftpunk97-zz
Copy link
Contributor

This PR contains fixes for the problems I encountered while preparing the demo for my JSoC report.
It fixes the following issues...

  • floor was applied to the low fields of the Box space, instead of ceil. This fixes an issue where we may occasionally get an out-of-bound value for something like Box(1.1, 3.4, Int32).

  • The spaces are seldom used directly by the user. Exporting them to main API interface, which should only be for the functions that are used on a constant basis, is not the right choice. Therefore, those exports have been removed, and the environments can call them by the using .Space statement

  • Removed the export of contains from Space module, as Base.in has already been overloaded. to call contains

  • Fixed an issue where calling the render!() function in :human_pane or :rgb mode would not display immediately in a script.

  • Switched the order the train and render_mode arguments in the definition of the make function. User will supply the value of the render_mode argument in more cases than for train, so train will be using its default value more often, and needs to be on the right side.

@kraftpunk97-zz
Copy link
Contributor Author

@tejank10 Committing a second batch of fixes. Here is a summary of the notable changes...

  • Reordered the arguments in the signature of contains to match it with Base.in. contains def of Spaces that are collections of other spaces was modified to use Base.in.

  • Tried to fix the Box equality issue as discussed in Box space equality is not correct #21. Two Box Spaces with different dtypes have been considered equal if they're Integers and either both are signed or both unsigned. Int16 Box space has been assumed to be equal to an Int8 box; reason being that they represent the same set of numbers, but using different number of bytes (UInt8(3) == UInt16(3) evaluates to true).

src/Spaces/box.jl Outdated Show resolved Hide resolved
@kraftpunk97-zz kraftpunk97-zz mentioned this pull request May 30, 2019
4 tasks
src/Spaces/box.jl Outdated Show resolved Hide resolved
@kraftpunk97-zz
Copy link
Contributor Author

kraftpunk97-zz commented Jun 1, 2019

@tejank10 added the requested changes. Please try to overlook the commit history mess. I ended up pushing the wrong commit to the wrong branch and that led to all kinds of trouble. Git wasn't being very nice.

@tejank10 tejank10 merged commit fcd5f67 into FluxML:master Jun 4, 2019
@tejank10
Copy link
Contributor

tejank10 commented Jun 4, 2019

Nice work, @kraftpunk97 !

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