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

Fixed outport bug and added tomography #41

Closed
wants to merge 8 commits into from
Closed

Fixed outport bug and added tomography #41

wants to merge 8 commits into from

Conversation

celiafish
Copy link
Contributor

No description provided.

@tacaswell
Copy link
Member

This needs to be rebased onto current master as there is a conflict that can not be auto-merged.


Parameters
----------
dataset : array_like
Copy link
Member

Choose a reason for hiding this comment

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

This is not correct, array-like objects do not have a method correct_drift

Copy link
Member

Choose a reason for hiding this comment

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

This note applies to basically every doc string ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The dataset is a XTomoDataset type, which is of course not one of existing types in VisTrails. To pass the format testing, I have to choose array_like. Do you know any better solution?

@celiafish
Copy link
Contributor Author

Let me try to fix the conflict.

@tacaswell
Copy link
Member

I think we either need to not use their dataset object and directly use the functions that do the work (my preference) and pass around ndarrays of the data, construct a new data set object for each function call and pass around the real data (annoying, but second place), or document these as what ever type of objects they really are and either expose that in VT as a module type or just use the 'variable' type (my least favorite).

Having a single mutable object that you make sequential calls on is very much against the view of the world that vistrails has (it is, at it's core, a functional language).

One of the design principles of skit-xray is to only use 'base' (core python + numpy) types as inputs/outputs of user-exposed functions.

@celiafish
Copy link
Contributor Author

In that sense, vistrails is unique being a functional language in the OO world.... I agree with that design principle of skit-xray about using only base types. So how about this: 1. let me first put variable type instead of array_like, and demo it to Wah-Keat and others (I need his opinion about the functional benefits i.e. is it what he wants, if he wants more functions, requirements, etc.). 2. modify the code and pass around just ndarrays. This strategy should make sure the tomo package is on the right track and also it is coded in the right way.

@tacaswell
Copy link
Member

From talking to Doga in April, I am pretty sure that there are clean functions underneath, the XTomoDataset objects mostly just does marshaling.

I think I will back the default value changes off of the wrap_extension branch (as it is broken for reasons I don't understand) so we can get that merged, which will make it easier to deal with arbitrary class mappings in VT.

@celiafish
Copy link
Contributor Author

It really depends on how much time to spend on digging out their data structure and understand their code. They did marshaling to make it more OO look. So by passing the XTomoDataset object, we won't have huge number of parameters associated with each function call. It is less frustrating in users perspective though. I'd say, for plan A, let's treat our usage of tomopy as application level. This way it gains more time for us to focus on the usability of these functions. Once this is OK, let's go for plan B, which we care as development team, but the beamline scientists don't. Let's talk in details when we meet next time.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.26%) when pulling 55f157c on celiafish:master into 35f5695 on Nikea:master.

@celiafish
Copy link
Contributor Author

After I merged, got a new error when loading NSLS-II module in vistrails:

File "/home/wxu/NSLS2/VTTools/vttools/to_wrap/fitting.py", line 36, in <module>
    from skxray.fitting.api import (QuadraticModel, GaussianModel,
ImportError: cannot import name ExpressionModel

Any idea?

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.26%) when pulling 55f157c on celiafish:master into 35f5695 on Nikea:master.

@tacaswell
Copy link
Member

You need to update skxray and probably lmfit.

Aren't moving parts fun?

@tacaswell
Copy link
Member

Both have conda packages built on my channel

@celiafish
Copy link
Contributor Author

Oops... after updating skxray, the same error showed up the other day shows up again...

OSError: /home/wxu/anaconda/envs/vt_bare/bin/../lib/libm.so.6: version `GLIBC_2.15' not found (required by /usr/local/lib/libfftw3f.so.3)

@tacaswell
Copy link
Member

sigh, try doing a conda remove system

@tacaswell
Copy link
Member

and you did some sudo install stuff while installing fftw or are you using system fftw?

@celiafish
Copy link
Contributor Author

Yes, I did sudo install. The fftw3f package seemed to be a separate one...

On Dec 11, 2014, at 6:58 PM, Thomas A Caswell notifications@github.com wrote:

and you did some sudo install stuff while installing fftw or are you using system fftw?


Reply to this email directly or view it on GitHub.

@tacaswell
Copy link
Member

We should avoid using anything with sudo a) to avoid clashing with the sysadmin on the beam line computers b) because not all users have sudo rights.

could you also try installing conda install glibc -c tacaswell? I re-compiled libc last night, I am curious if it fixes your problem. I suspect it won't as it will still be the wrong version of libm.... (but a newer one!)

@celiafish
Copy link
Contributor Author

You can't imagine how much time I spent on installing tomopy.... Sure will try that.

On Dec 11, 2014, at 9:56 PM, Thomas A Caswell notifications@github.com wrote:

We should avoid using anything with sudo a) to avoid clashing with the sysadmin on the beam line computers b) because not all users have sudo rights.

could you also try installing conda install glibc -c tacaswell? I re-compiled libc last night, I am curious if it fixes your problem. I suspect it won't as it will still be the wrong version of libm.... (but a newer one!)


Reply to this email directly or view it on GitHub.

@tacaswell
Copy link
Member

We should also avoid inflicting that amount of pain on our users ;)

@celiafish
Copy link
Contributor Author

Tried install glibc. Now have segmentation fault...

@tacaswell
Copy link
Member

@celiafish Is this different than your other PR?

@celiafish
Copy link
Contributor Author

This was an old PR. The useful one is fixing outport bug part. Other than that you can simply ignore this one.

@tacaswell tacaswell closed this Feb 25, 2015
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

4 participants