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

improve pretty print for DimensionalArray #33

Merged
merged 5 commits into from Jan 17, 2020
Merged

improve pretty print for DimensionalArray #33

merged 5 commits into from Jan 17, 2020

Conversation

Datseris
Copy link
Contributor

@Datseris Datseris commented Jan 13, 2020

Now from

using Dates, DimensionalData
using DimensionalData: Time, X
timespan = DateTime(2001):Month(1):DateTime(2001,12)
t = Time(timespan)
x = X(10:10:100)
A = DimensionalArray(rand(12,10), (Time(timespan), X(10:10:100)))

one gets:
image

@Datseris
Copy link
Contributor Author

I don't know if this is a bug, unwanted behavior, or wanted behavior, but this PR made me realize that:

julia> B = mean(A; dims = Time)
DimensionalArray with dimensions:
  Time: DateTime[2001-01-01T00:00:00]
  X: 10:10:100
and data:
1×10 Array{Float64,2}:
 0.573145  0.576649  0.454423  0.528486  0.639443  …  0.373484  0.464725  0.47384  0.526452

reducing operations keep the dimensions even if reduced over them (in contrast to xarray from Python).

@codecov-io
Copy link

codecov-io commented Jan 13, 2020

Codecov Report

Merging #33 into master will increase coverage by 1.04%.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #33      +/-   ##
=========================================
+ Coverage   88.15%   89.2%   +1.04%     
=========================================
  Files          10      11       +1     
  Lines         515     528      +13     
=========================================
+ Hits          454     471      +17     
+ Misses         61      57       -4
Impacted Files Coverage Δ
src/dimension.jl 91.83% <ø> (-1.27%) ⬇️
src/DimensionalData.jl 100% <ø> (ø) ⬆️
src/array.jl 84.84% <100%> (+16.55%) ⬆️
src/prettyprint.jl 86.2% <86.2%> (ø)
src/methods.jl 95.12% <0%> (+0.06%) ⬆️

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 847eb58...5a8ec98. Read the comment docs.

@rafaqz
Copy link
Owner

rafaqz commented Jan 13, 2020

That's because Julia keeps them! After a reducing operation you always have a length 1 dim. So we have to keep the dimension type too... I just change the step size and make it cover the whole original dim length.

I'm not sure what python does.

@rafaqz
Copy link
Owner

rafaqz commented Jan 13, 2020

But otherwise that looks great. You might want to try it with a large vector of DateTime or something as val and see if it holds up - we might need to just use the start and end however julia does that with arrays.

Also maybe try it with larger grids.

@Datseris
Copy link
Contributor Author

You might want to try it with a large vector of DateTime or something as val and see if it holds up - we might need to just use the start and end however julia does that with arrays.

Good point and thankfully I know how to fix it since I've already done it in DynamicaSystems.jl. I'll check it tomorrow at the office.

@Datseris
Copy link
Contributor Author

That's because Julia keeps them! After a reducing operation you always have a length 1 dim. So we have to keep the dimension type too... I just change the step size and make it cover the whole original dim length.

Yeah, you are right (although I have to admit I never liked that Julia decision). Regardless, what I do personally is implement drop versions of these functions, like dropmean or dropsum that just do

dropmean(A; dims = 1) = dropdims(mean(A; dims = dims); dims = dims)

As you can see, for standard Julia arrays this is no big deal. But for DimensionalArray, I would imagine that the processes of "dropping" the singleton dimension is much more involved for the general user. Perhaps I should contribute dropmean/sum/etc. here?

Python's xarray automatically drops singleton dimensions, e.g. if you sum over Lattitude, the final array will have only longitude.

@rafaqz
Copy link
Owner

rafaqz commented Jan 13, 2020

dropdims is actually easier with dimensions as you don't need to know the number: dropdims(A; dims=X)

Look in the methods.jl tests to see everything that's implemented with dims. It's quite a lot.

For your dropmean I actually want to minimise custom method exports and focus on covering all the base methods first. DimensionalArrays should feel pretty much the same as Arrays just with typed dims.

@Datseris
Copy link
Contributor Author

running test/prettyprint.jl now gives:

image

Unfortunately, even though I've used the limiter function suggested by Sebastian, it works but not "precisely". It limits the vector output to a elements to fill the row, as it should, but its just goes a little overboard in Juno. Regardless, this is unrelated with this PR, and as far as Julia Base is concerned, I used all the correct tools to make it print the dimension values in 1 line. The rest is up to Juno, and I've reported it.

This is good to go from my end.

timespan = DateTime(2001):Month(1):DateTime(2001,12)
t = Time(timespan)
x = X(Vector(10:10:500))
A = DimensionalArray(rand(12,length(x)), (t, x))
Copy link
Owner

Choose a reason for hiding this comment

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

Can we run show on these? We probably don't need an actual test, but it would be good to run the code to make sure it doesn't error.

@rafaqz
Copy link
Owner

rafaqz commented Jan 14, 2020

The outputs look great. Thanks for working on the print limiting I'm happy with that.

@Datseris
Copy link
Contributor Author

The outputs look great. Thanks for working on the print limiting I'm happy with that.

Thanks, you are welcome. Can I ask why you didn't merge it if you liked the PR? Seems pretty useful to have and I'd like to work on other features as well.

@rafaqz
Copy link
Owner

rafaqz commented Jan 16, 2020

See the inline comment above! Maybe you missed it. Just add show methods in the tests to make sure they don't error, as a minimal test.

@Datseris
Copy link
Contributor Author

Ah sorry, yes I missed it. I can add proper tests, hold on.

@rafaqz
Copy link
Owner

rafaqz commented Jan 16, 2020

Thanks thats even better

@rafaqz rafaqz merged commit 3189afa into rafaqz:master Jan 17, 2020
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

3 participants