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

parametric NDArray #331

Merged
merged 40 commits into from
Dec 1, 2017
Merged

parametric NDArray #331

merged 40 commits into from
Dec 1, 2017

Conversation

iblislin
Copy link
Member

No description provided.

src/executor.jl Outdated
G<:AbstractVector{<:Union{Void,NDArray}},
O<:VecOfNDArray,
D<:Dict{Symbol,<:NDArray},
E<:Dict{Symbol,<:NDArray}}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is terrible, any better way to deal with abstract container type?
Or we need to redesign Executor?

@iblislin iblislin force-pushed the ib/param-nd branch 2 times, most recently from c9cdd65 to d49aa7a Compare November 19, 2017 08:36
@iblislin
Copy link
Member Author

with parametric NDArray, constructing Dict will easily get Dict{Symbol, Any}. Is this ok for us?
Before parametric, we have lots of functions (like bind) constrain some input type as Dict{Symbol, NDArray}.

julia> Dict(:x => x, :y => y)                                                                                                                     
Dict{Symbol,Any} with 2 entries:                                                                                                                  
  :y => 6 mx.NDArray{Float32,1} @ CPU0:                                                                                                          
  :x => 2×3 mx.NDArray{Float32,2} @ CPU0:

@pluskid
Copy link
Member

pluskid commented Nov 22, 2017

hmm, I would like to avoid being resolved to Any if possible.

@iblislin
Copy link
Member Author

Actually, same story on Base.Array

julia> x = [1,2]
2-element Array{Int64,1}:
 1
 2

julia> y = [1 2; 3 4]
2×2 Array{Int64,2}:
 1  2
 3  4

julia> Dict(:x => x, :y => y)
Dict{Symbol,Any} with 2 entries:
  :y => [1 2; 3 4]
  :x => [1, 2]

src/metric.jl Outdated
mutable struct MSE <: AbstractEvalMetric
mse_sum :: Vector{NDArray}
mutable struct MSE{T<:NDArray} <: AbstractEvalMetric
mse_sum :: Vector{T}
Copy link
Member Author

Choose a reason for hiding this comment

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

@vchuravy I changed MSE like this, but I got confused that how does user initialize a MSE instance?
In the example of regression, it just creates a instance via MSE().
And...what's your design of struct MSE if we have parametric NDArray?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is for error reporting only it should simply default to Float64 or Float32.
The array is simply to not block during _update_single_output.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since this is for error reporting only it should simply default to Float64 or Float32.

okay, I will init them with MX_float (Float32) by default.

The array is simply to not block during _update_single_output.

I do not get what you mean.

@iblislin iblislin closed this Nov 24, 2017
@iblislin iblislin deleted the ib/param-nd branch November 24, 2017 14:21
@iblislin iblislin restored the ib/param-nd branch November 24, 2017 14:21
@iblislin iblislin reopened this Nov 24, 2017
@codecov-io
Copy link

codecov-io commented Nov 24, 2017

Codecov Report

Merging #331 into master will increase coverage by 0.07%.
The diff coverage is 72.51%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #331      +/-   ##
==========================================
+ Coverage   70.01%   70.08%   +0.07%     
==========================================
  Files          24       24              
  Lines        1944     1939       -5     
==========================================
- Hits         1361     1359       -2     
+ Misses        583      580       -3
Impacted Files Coverage Δ
src/metric.jl 29.1% <10.52%> (ø) ⬆️
src/callback.jl 100% <100%> (ø) ⬆️
src/util.jl 33.98% <100%> (+4.08%) ⬆️
src/kvstore.jl 75% <50%> (ø) ⬆️
src/io.jl 80.76% <66.66%> (ø) ⬆️
src/model.jl 70.42% <76.92%> (+0.11%) ⬆️
src/executor.jl 84.33% <82.35%> (+0.24%) ⬆️
src/ndarray.jl 85.22% <86.88%> (+0.26%) ⬆️

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 2a5a284...1571255. Read the comment docs.

@iblislin
Copy link
Member Author

This PR is ready for review now.

@iblislin iblislin changed the title WIP: parametric NDArray parametric NDArray Nov 26, 2017
@iblislin
Copy link
Member Author

good to go ?

@iblislin iblislin merged commit cb042fd into master Dec 1, 2017
@iblislin iblislin deleted the ib/param-nd branch December 1, 2017 13:18
@iblislin iblislin added this to the 0.4.0 milestone Dec 1, 2017
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