search

wkearn / RasterIO.jl

Simple Raster Formats for Julia
Other
18 stars 5 forks source link

testing git... + gdalinfo #12

Closed marciolegal closed 9 years ago

marciolegal commented 9 years ago

Hi, I'm still geting the hang of git (hope it gets replaced with something less crypt someday..). As a first pull request test, i'm trying to propose a simple function to display raster info from the raster disk path, using some of the function provided by @yeesian : gdalinfo. Please take a look. Best, M.

yeesian commented 9 years ago

Thanks for your first PR! A few comments:

  1. It is non-standard for there to be print statements within a library function
  2. I don't yet see where you're going with this. Do you intend to follow-up with the rest of the gdal apps? If so, they might be better organized under an examples/ or apps/ folder.

More importantly, If you have the path to a raster file, it is unclear to me why you should call gdalinfo() inside a (presumably interactive) julia session, when you can "shell out" and call gdalinfo on the file itself directly?

This work might be better suited for the REPL display of a Raster object, by implementing Base.print and Base.show (e.g. in Proj4.jl)?

marciolegal commented 9 years ago

does this line actually work?

I didnt have a case where openraster succeeds but returns an empty dataset, so I couldnt really test it. Maybe shoud replace by if (raster.dataset == C_Null) ?

I don't yet see where you're going with this. Do you intend to follow-up with the rest of the gdal apps? If so, they might be better organized under an examples/ or apps/ folder.

What do you think? As a user, I find some of these apps very helpfull, so we could at least implement some of their functionally in an app folder like you suggested?

More importantly, If you have the path to a raster file, it is unclear to me why you should call gdalinfo() inside a (presumably interactive) julia session, when you can "shell out" and call gdalinfo on the file itself directly?

This turns the function available from within the Julia environment, and I guess can be helpfull to people using software like Juno, that accepts commands in a "non-shellable" editor and prints outputs in a console-like window. Another reason is, in windows, this avoids having to add the gdal apps folder to the path to be able to use the .exe files (after providing the right path to gdallib). In addition, it is a good thing to have a quick way to check a raster specs before opening it. But yes, this could and should accept as input a raster object.

marciolegal commented 9 years ago

This work might be better suited for the REPL display of a Raster object, by implementing Base.print and Base.show (e.g. in Proj4.jl)?

Ok, I didn't know that. I will do it in my next try : P

yeesian commented 9 years ago

Maybe shoud replace by if (raster.dataset == C_Null) ?

Try things out in the terminal when unsure (:

julia> C_Null
ERROR: UndefVarError: C_Null not defined

julia> C_NULL
Ptr{Void} @0x0000000000000000

As a user, I find some of these apps very helpfull, so we could at least implement some of their functionally in an app folder like you suggested?

If you insist, you can maintain a personal/separate repository of such utility functions for the time being. This being your first PR, I think you should keep it small and focus on REPL display/printing for the Raster objects.

Many of the apps will require you to wrap the rest of GDAL/OGR anyway.

marciolegal commented 9 years ago

How do you implement a function that changes what is shown in the REPL when you type an object and press Enter? For example, when I do myraster= openraster(etc) then myraster shows something like RasterIO.Raster(Ptr{Void} @...... I would like it to show something like "1000 x 1000 x 2 RasterIO.Raster". I tried implementating show() and print(), but I only works when typing show(myraster) or print(myraster). If i do just myraster, it only shows as RasterIO.Raster(Ptr{Void} @.....

yeesian commented 9 years ago

you'll need to overwrite the Base method(s). See http://stackoverflow.com/questions/17029695/use-show-to-print-output

garborg commented 9 years ago

I think it's cleaner to extend without importing (unless perhaps if the file contained dozens of extensions and nothing else), e.g. 'function Base.show(io, x::T) ...'

On Thursday, September 24, 2015, Yeesian Ng notifications@github.com wrote:

you'll need to import the Base methods first. See http://stackoverflow.com/questions/17029695/use-show-to-print-output

— Reply to this email directly or view it on GitHub https://github.com/wkearn/RasterIO.jl/pull/12#issuecomment-143021744.