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

added STL datatype #2679

Merged
merged 24 commits into from Jan 5, 2017

Conversation

Projects
None yet
7 participants
@ThomasWollmann
Copy link
Contributor

commented Jul 28, 2016

No description provided.

@ThomasWollmann

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2016

Added image support to IEs as well. Do you mind if I don't open a second pull request for this minor change?

<datatype extension="plybinary" type="galaxy.datatypes.constructive_solid_geometry:PlyBinary" display_in_upload="true" />
<datatype extension="vtkascii" type="galaxy.datatypes.constructive_solid_geometry:VtkAscii" display_in_upload="true" />
<datatype extension="vtkbinary" type="galaxy.datatypes.constructive_solid_geometry:VtkBinary" display_in_upload="true" />
<datatype extension="stl" type="galaxy.datatypes.constructive_solid_geometry:STL" display_in_upload="True"/>

This comment has been minimized.

Copy link
@bgruening

bgruening Jul 28, 2016

Member

s/True/true just for being consistent.

@@ -69,3 +69,5 @@ ecdsa==0.13
# Flexible BAM index naming
pysam==0.8.4+gx1

# CSG dependencies
numpy-stl==1.8.0

This comment has been minimized.

Copy link
@bgruening

bgruening Jul 28, 2016

Member

Thomas for generating a wheel that is automatically shipped with Galaxy you need to create a wheel on our build-server with https://github.com/galaxyproject/starforge

Example PR is for example here: galaxyproject/starforge#100

This comment has been minimized.

Copy link
@bgruening

bgruening Jul 28, 2016

Member

We should have now a wheel for version 1.9.1
galaxyproject/starforge#104
Can you adjust this and then we are ready to go.

ThomasWollmann added some commits Jul 28, 2016

@@ -110,7 +111,7 @@ def display_peek(self, dataset):

class PlyAscii(Ply, data.Text):

file_ext = "plyascii"
file_ext = "ply"

This comment has been minimized.

Copy link
@bgruening

bgruening Jul 28, 2016

Member

@gregvonkuster do you see any negative impact in changing this?

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jul 28, 2016

Member

Yes, this and the following changes are wrongly assigning the same value to the file_ext of different classes: this attribute should be unique and equal to what is used as extension in config/datatypes_conf.xml.sample .

This comment has been minimized.

Copy link
@gregvonkuster

gregvonkuster Jul 28, 2016

Contributor

@nsoranzo is correct - there are binary and ascii types of the ply datatype, so we need to differentiate them.

This comment has been minimized.

Copy link
@ThomasWollmann

ThomasWollmann Jul 28, 2016

Author Contributor

Still many tools don't distinguish between the extensions. Havn't seen any tool which saves a file in xxx.vtkascii. A correct file extension is mandatory for https://github.com/ThomasWollmann/docker-paraviewweb.

This comment has been minimized.

Copy link
@bgruening

bgruening Jul 28, 2016

Member

Thomas, for the time being can you leave the extensions as they are and use in paraview a if condition like

#if $hda.ext in ['plybinary', 'plyascii']:
   cp $hda foo.ply 
#end 

This comment has been minimized.

Copy link
@ThomasWollmann

ThomasWollmann Jul 28, 2016

Author Contributor

Very dirty, but I'll do.

@galaxybot galaxybot added the triage label Jul 28, 2016

@galaxybot galaxybot added this to the 16.07 milestone Jul 28, 2016

ThomasWollmann added a commit to BMCV/galaxy-image-analysis that referenced this pull request Jul 28, 2016

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Jul 28, 2016

@galaxybot test this

@bgruening

This comment has been minimized.

Copy link
Member

commented Aug 4, 2016

@ThomasWollmann the good point is we are now using the numpy-stl wheel and you can rely on it. The bad part is that your sniffer is not specific enough. Other files are also recognised as stl: https://travis-ci.org/galaxyproject/galaxy/jobs/149693586#L1166

@ThomasWollmann

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2016

Since it is not trivial for the binary STLs I used the library. Sadly it is not a working solution. I'll think about a new solution.

@bgruening

This comment has been minimized.

Copy link
Member

commented Aug 4, 2016

Maybe it's enough to check actually the content of your mesh. I assume that reading a text file or an other hdf5 store simply do not raise an exception, but the content is probably empty.

ThomasWollmann added some commits Aug 5, 2016

Update constructive_solid_geometry.py
added additional check to refine stl sniffer
@@ -8,6 +8,9 @@
from galaxy.datatypes.metadata import MetadataElement
from galaxy import util

from stl import mesh
import numpy as np

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Aug 5, 2016

Member

Can you invert this 2 imports and move them before the galaxy imports? I.e.:

import numpy as np
from stl import mesh

from galaxy.datatypes import data
...

We are now following the smarkets import style, as in this example.

@jmchilton

This comment has been minimized.

Copy link
Member

commented Aug 25, 2016

I'm going to mark this as WIP until the above comments are resolved. Thanks for the initial crack at it and good luck improving the sniffer.

@ThomasWollmann

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2016

Still unsure if this is possible. Should I remove the sniffer so we can use the datatypes without automatic detection?

@bgruening

This comment has been minimized.

Copy link
Member

commented Oct 29, 2016

Yes, let's remove the sniffer.

ThomasWollmann added some commits Nov 14, 2016

Merge branch 'dev' of https://github.com/galaxyproject/galaxy into dev
# Conflicts:
#	config/datatypes_conf.xml.sample

@martenson martenson modified the milestones: 17.01, 16.10 Nov 16, 2016

@bgruening

This comment has been minimized.

Copy link
Member

commented Dec 16, 2016

Looks good to me. Hopefully we will find a way to sniff this in the future.
@nsoranzo what do you think can we merge this?

ThomasWollmann added some commits Dec 31, 2016

@bgruening

This comment has been minimized.

Copy link
Member

commented Jan 5, 2017

Thanks @ThomasWollmann!

@bgruening bgruening merged commit a870ac8 into galaxyproject:dev Jan 5, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@martenson martenson removed the status/WIP label Jul 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.