search

holoviz-topics / EarthSim

Tools for working with and visualizing environmental simulations.
https://earthsim.holoviz.org
BSD 3-Clause "New" or "Revised" License
65 stars 21 forks source link

Meshing update #308

Closed kcpevey closed 5 years ago

kcpevey commented 5 years ago

removes filigree dependency and replaces with xmsmesh

philippjfr commented 5 years ago

Just a few questions and requests:

  1. Should we also delete filigree.py?
  2. Could you update doc/user_guide/index.rst replacing Specifying_Meshes with Generating_Meshes? 3. Should we move the classes you've written into a new xmsmesh.py?

And one minor observation, the changes to requirements.txt are unnecessary now but won't do any harm.

philippjfr commented 5 years ago

Does this supersede #298?

kcpevey commented 5 years ago

Does this supersede #298?

Yes, I had a merge conflict and then managed to make a mess of things and I wasn't sure how to undo my mess so I just started over...

kcpevey commented 5 years ago

Whenever I use python setup.py develop with earthsim, pip doesn't recognize some of the required packages that ARE ALREADY in my env. So it ends up double installing things like Bokeh and it trashes my env because then both conda and pip are confused about what its in my env and I can't remove them without both of them getting confused. Have you seen this?

jbednar commented 5 years ago

I think it's safer to use python setup.py develop --no-deps or pip install -e . --no-deps to avoid pulling in bogus installations like that.

kcpevey commented 5 years ago

@jbednar Is that standard practice? It isn't something we can fix in the setup.py file?

philippjfr commented 5 years ago

I always use --no-deps with conda personally, not entirely sure why it doesn't recognize the dependencies but we should really never allow dependencies to be satisfied by setup.py develop so we should recommend that regardless.

kcpevey commented 5 years ago

@philippjfr I'm moving the classes into an xmsmesh.py file. Unfortunately I just trashed my env so I have create a new one before I can test. I'll ping you when I'm ready for review.

kcpevey commented 5 years ago

And one minor observation, the changes to requirements.txt are unnecessary now but won't do any harm.

I didn't change requirements.txt, which I dont think exists anymore, but I did update dependencies.txt. Why would that be unnecessary?

philippjfr commented 5 years ago

Sorry got confused, but dependencies.txt is also no longer really necessary because dependencies the authoritative source for dependencies is now setup.py. That said the developer instructions probably need updating to describe how to use that to set up an env, which basically just boils down to this command:

doit env_create -c pyviz/label/earthsim -c pyviz/label/dev -c defaults -c erdc -c conda-forge --name=earthsim --python=3.6
philippjfr commented 5 years ago

That also means you should add all those xms packages to the setup.py

kcpevey commented 5 years ago

It looks like the whole Getting Started page needs to be reworked to convey that there is a conda package available for general users and the command you listed above for developers.

kcpevey commented 5 years ago

@philippjfr @jbednar If you have strong feelings about how the getting started page gets worded, then I'll leave that to you. Otherwise, I don't mind revising it.

philippjfr commented 5 years ago

If you have strong feelings about how the getting started page gets worded, then I'll leave that to you. Otherwise, I don't mind revising it.

I'd be happy for you to make an initial cut and then we can make any edits later.

kcpevey commented 5 years ago

If you install from conda install -c conda-forge earthsim you don't need the repo and doit command, right?

philippjfr commented 5 years ago

If you install from conda install -c conda-forge earthsim you don't need the repo and doit command, right?

Correct, at least once we've finally released again.

kcpevey commented 5 years ago

@philippjfr ready for review.

I reorganized the getting started page, but you should probably go over those instructions closely and make sure that I have everything right.

philippjfr commented 5 years ago

The updates look good to me. I'll do some testing soon and then hopefully cut a release by Monday.

kcpevey commented 5 years ago

Almost forgot, python -c "import earthsim ; earthsim examples" wouldn't work for me when I got the package from conda. The only other solution I could see would be to launch that command from the environment folder. Certainly there is a better command?

philippjfr commented 5 years ago

Oh yeah, python -c "import earthsim ; earthsim examples" doesn't seem right, shouldn't that simply be earthsim examples?

kcpevey commented 5 years ago

even with earthsim in my environment, it wouldn't recognize the command earthsim. That command only works from inside of the repository folder.

kcpevey commented 5 years ago

That becomes an issue when you've installed it via conda instead of the repo

philippjfr commented 5 years ago

python -m "earthsim examples" then.

philippjfr commented 5 years ago

That becomes an issue when you've installed it via conda instead of the repo

I'll fix that too.

philippjfr commented 5 years ago

Tests are failing because the aquaveo channel needs to be added to the .travis.yml and appveyor.yml to satisfy the xms dependencies.

kcpevey commented 5 years ago

My apologies. A lot of this is new to me!

philippjfr commented 5 years ago

My apologies. A lot of this is new to me!

No problem, thanks for being so patient and making all these changes!

philippjfr commented 5 years ago

@kcpevey Do you know if aquaveo will make OSX packages for xmsmesh & co. available?

philippjfr commented 5 years ago

Actually, they do appear to exist, not sure why travis isn't picking up on them.

philippjfr commented 5 years ago

Thanks @kcpevey! Glad to have gotten rid of the filligree dependency.