by

Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Activity
  • 02:18
    morganjwilliams commented #37
  • 02:01
    morganjwilliams closed #37
  • 02:00
    morganjwilliams synchronize #37
  • Jul 06 16:23
    kaarelmand commented #37
  • Jul 06 16:23
    kaarelmand commented #37
  • Jul 06 16:14
    kaarelmand commented #37
  • Jul 06 16:11
    kaarelmand synchronize #37
  • Jul 06 16:07
    kaarelmand synchronize #37
  • Jul 06 07:40
    morganjwilliams commented #37
  • Jul 06 06:48
    morganjwilliams commented #37
  • Jul 04 01:55
    kaarelmand commented #37
  • Jul 04 01:55
    kaarelmand commented #37
  • Jul 04 01:53
    kaarelmand edited #37
  • Jul 04 01:53
    kaarelmand synchronize #37
  • Jul 04 01:50
    kaarelmand edited #37
  • Jul 04 01:46
    kaarelmand synchronize #37
  • Jul 04 01:02
    kaarelmand edited #37
  • Jul 03 05:13
    kaarelmand commented #37
  • Jul 03 05:00
    kaarelmand edited #37
  • Jul 03 04:59
    kaarelmand edited #37
Morgan Williams
@morganjwilliams
Then there's only one call to the function - the issue arises through repetitive use. I'm not sure exactly where the memory leak is, but the function could definitely be improved.
Morgan Williams
@morganjwilliams
@kaarelmand Checking out the first issue now - pyrolite.data.mineral wasn't in the MANIFEST.in which controls which files get included in the install, so you did indeed find a bug!
Should work in the next release, or alternatively develop after morganjwilliams/pyrolite@4ecb157
Morgan Williams
@morganjwilliams
@kaarelmand I'll dig into morganjwilliams/pyrolite#29 more tomorrow (I hope you managed to get around it in the meantime!), I had morganjwilliams/pyrolite#39 come in which deserved some attention.
Morgan Williams
@morganjwilliams
I think I've tracked down the source of #29, and have now addressed this with morganjwilliams/pyrolite@18fede0. I'll look at revamping some of these functions to make them more readable, and then merge this branch into develop.
Kaarel Mänd
@kaarelmand
Oh wow, thanks for handling the bug! I was able to work past it, yes, but this will help me cut down on quite a few lines. Sorry for being off the radar for a bit. I'll test it out tomorrow.
Morgan Williams
@morganjwilliams
No worries. If you want to test it out, make sure you pull down that feature branch: convert_chemistry_revamp
Kaarel Mänd
@kaarelmand
Aha, thanks for the heads up. Still wrapping my head around the whole git landscape.
Morgan Williams
@morganjwilliams
No worries. Feel free to ping me if you run into issues. It'll be merged back into develop soon (I'll use the opportunity to clean up a few of the geochem functions too), and in the release for 0.2.6.
Kaarel Mänd
@kaarelmand
That commit works for me! My spreadsheet stitching program runs now without the workaround. Pretty neat, thanks!
Morgan Williams
@morganjwilliams
Great! :smile:
Kaarel Mänd
@kaarelmand
On another matter, I should really finish up my pull request about those extra reference compositions. I actually meant to ask about the source .csv files: does it matter at all whether I input the compositions as oxides or elements?
And does it help at all if the same element is reported both as an element and oxide?
I guess it would all get automatically converted anyway?
Morgan Williams
@morganjwilliams
If you report it as both, it'll end up doubled.
Kaarel Mänd
@kaarelmand
Oh, so that's bad.
Morgan Williams
@morganjwilliams
It doesn't matter whether you add it as oxides or elements - generally I'd suggest input it as it was in the reference to be precise.
Yeah. Anything but duplication is good.
Kaarel Mänd
@kaarelmand
Yep, I'll, uh, have to make some modifications to the currently committed compositions.
Morgan Williams
@morganjwilliams
Situations like where you have FeO and Fe2O3 (e.g. where you know the Fe2+/Fe3+ ratio) are OK, but then it's more about understanding redox/species distribution
Kaarel Mänd
@kaarelmand
Okay; I think these reference comps usually don't do that.
Morgan Williams
@morganjwilliams
Yeah, it's rare that redox pairs are specified. Some of the basalt ones might have it.
Kaarel Mänd
@kaarelmand
I'll work on getting those comps added soon then, not having to convert all the chemistry makes it less tedious at least. I also started writing up a documentation section a while ago, so make the final push on that one, too.
Morgan Williams
@morganjwilliams
Ok, great! I've been thinking how to better incorporate bibliographic data into these files, and will think about putting up a composition template to make this easier soon too.
Kaarel Mänd
@kaarelmand
That would make it easier, sure. Is that anything that pandoc-citeproc can help with? They have a neat markdown addition where you just need to write [@some_bibtex_header] to get a nicely formatted citation.
Morgan Williams
@morganjwilliams
The docs are largely in RST, and dealing with markdown and a bibliography across platforms/github could be tricky for the citations.
Kaarel Mänd
@kaarelmand
Okay, nevermind! :) Thought they were markdown for some reason.
Well, I'll pop by more often now, I hope. Thanks for the help and see ya! Be safe!
Morgan Williams
@morganjwilliams
We can get OK citations even in docstrings (e.g. the lattice strain functions)
Sounds good, stay well!
Morgan Williams
@morganjwilliams
I've merged the convert_chemistry branch back into develop - the fix for #29 should now be more accessible while I work on a few other things in the meantime.
Morgan Williams
@morganjwilliams
A bugfix for morganjwilliams/pyrolite#39 has also now been merged into develop. The pyrolite lambdas functions should now give comparable results to O'Neill (2016). With this, there's also a new reference composition (ChondriteREE_ON) with the REE abundances from the paper, which is used by default. Thanks to Laura Miller for the heads up on this one.
Morgan Williams
@morganjwilliams

I'll release v0.2.6 this week, incorporating all recent updates on develop and a few items raised by the pyOpenSci/software-review#20.

@kaarelmand let me know if you're keen to finish off the PAAS pull request for this release, or it can wait for 0.2.7/0.3.0 (there's a good chance the next release will have a quicker turnaround).

lithophilipp
@lithophilipp
Hi Morgan. I finally got to check out pyrolite (or am just about to begin). However, when I tried to run your TAS.py template, an error message occurred. Might be a simply user error from my side (new to python) or is this an apple vs. windows thing? I
NotImplementedError: cannot instantiate 'WindowsPath' on your system
Kaarel Mänd
@kaarelmand
Hi, sorry for the interminable delay with the PAAS PR. Please don't delay the release on that account. Unfortunately I have to really focus on writing my thesis for now...
Morgan Williams
@morganjwilliams
@kaarelmand no worries, I put out that release, and no rush on PAAS for now. 🙂
Morgan Williams
@morganjwilliams
@lithophilipp welcome! It could be either, or an error on my side - if you could send through any more details that'd be handy (either here or via email as suits; e.g. Have you edited the example at all, and which line is the error thrown on?). I'll have a quick look to see if I can spot what might have gone wrong today.
Morgan Williams
@morganjwilliams
I think I might know where it may have tripped up (if so, it should be a quick fix), but will wait for some more info.
Morgan Williams
@morganjwilliams
Just a quick note that I've added an issue to address the TAS on Mac issue morganjwilliams/pyrolite#44. @lithophilipp
Morgan Williams
@morganjwilliams
As of this evening, pyrolite is officially peer-reviewed and published! https://joss.theoj.org/papers/10.21105/joss.02314 :tada:
Morgan Williams
@morganjwilliams
@lithophilipp I've make a few changes to address some of the changes with e.g. the TAS classifier. If you'd like to try out the recent changes, you can install the development version, otherwise the changes will be out with the next release (0.2.8). I've also added automated tests to which run on OSX, which will hopefully allow us to identify mac-related issues easier if they pop up.
Kaarel Mänd
@kaarelmand
Belated hurray on the publication, that's really great to hear! 🎉🎉🎉
Morgan Williams
@morganjwilliams
Cheers @kaarelmand! Also thanks for pushing a few updates to your PR - let me know when you're happy to merge this in. Looking at another release soon, but will be enough time to merge, check and test beforehand if you'd like to finish it off.
Kaarel Mänd
@kaarelmand
This one should be done, now! Not sure if I need to do anything more, like run tests and such? Also, I have to figure out how to bring my local copy of pyrolite up to date now...
Morgan Williams
@morganjwilliams
Ok, I'll have a look shortly. If you want to continue with your local install, you could update your fork's develop branch (e.g. roughly following https://stackoverflow.com/questions/7244321/how-do-i-update-a-github-forked-repository). Otherwise once the PR is merged can go back to using a fresh normal (or development) pyrolite install.
Morgan Williams
@morganjwilliams
@kaarelmand I added a few comments to the PR regarding a few quick changes - once those are done I'll merge it into develop. Cheers!
Kaarel Mänd
@kaarelmand
Thanks for the review, fixes should be committed now. And thanks for that reference on updating!
Kaarel Mänd
@kaarelmand
By the way, I've written short paragraphs explaining the idiosyncrasies and uses of the various shale/mud reference compositions I added here. Should I open a PR where I add them to docs/source/data/refcomp.rst?
Morgan Williams
@morganjwilliams
@kaarelmand for the descriptions etc hold on a short while until morganjwilliams/pyrolite#38 is addressed - then that information can go there. If you have ideas on what should go on the page (or the logistics of what should go in the files/templates/file formats and how that's automatically imported to that page) feel free to comment on that issue specifically.