search

insarlab / PySolid

A Python wrapper for solid Earth tides
GNU General Public License v3.0
61 stars 7 forks source link

create unique text file for solid.for to write to, fix #50 #55

Closed scottstanie closed 1 year ago

scottstanie commented 1 year ago

Seems to fix the race condition which happens when multiple processes are running in the same directory that @vbrancat and I also saw, which I see was already opened in #50

I started on directly returning arrays from fortran but gave up on it. Maybe some day...

scottstanie commented 1 year ago

now that i've fixed the failing CIs, I'll confirm that it does seem to fix what I was running (which was 6 COMPASS runs at a time)

$ find run_files -type f | xargs --max-procs=6 --max-args=1 bash

...
PYSOLID: read data from text file: /tmp/pysolid_hlm_ffdn.txt

...
PYSOLID: read data from text file: /tmp/pysolid_a4kkhmtd.txt

note that i also got a warning, which can be a separate issue:

/u/aurora-r0/staniewi/repos/PySolid/src/pysolid/grid.py:92: UserWarning: Input line 1 contained no data and will not be counted towards `max_rows=2600`.  This differs from the behaviour in NumPy <=1.22 which counted lines rather than rows.  If desired, the previous behaviour can be achieved by using `itertools.islice`.
Please see the 1.23 release notes for an example on how to do this.  If you wish to ignore this warning, use `warnings.filterwarnings`.  This warning is expected to be removed in the future and is given only once per `loadtxt` call.
  fc = np.loadtxt(txt_file,
yunjunz commented 1 year ago

note that i also got a warning, which can be a separate issue:

That's an existing warning from numpy, nothing to do with pysolid, you could safely ignore it.

scottstanie commented 1 year ago

Thanks @yunjunz ! So I was going to make these small fixes here... I did some quick fortran searching and got the subroutines to return the arrays directly without writing to a file in #56

Would you still want this PR since it's simpler? Or should we just go to the better solution in that other PR?

yunjunz commented 1 year ago

That's great news @scottstanie! I would like both if it's not too much to ask~ The current PR is a simpler change, as you said, I would like to have this in the commit history.

scottstanie commented 1 year ago

that makes sense too! i'll rebase that other PR off of this one once it's merged in.