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

Loadtxt real format update to list directed #805

Merged
merged 8 commits into from
May 13, 2024

Conversation

chuckyvt
Copy link
Contributor

This is a minor change to the loadtxt function to be more robust based on my experience in 'real world' use. Currently loadtxt specifies a format for the real read function, which makes it 'brittle'. If the text format is too far off, the read fails. Changing to list directed read is more robust in my use over the last few months in the version I build and use. The existing test for loadtxt is a good functional test but doesn't do a good job of simulating actual use, as by definition the text input and read format are defined to be identical.

Change loadtxt format for real numbers to list directed.
@jvdp1
Copy link
Member

jvdp1 commented Apr 25, 2024

this format has been introduced by @milancurcic with this commit. This PR would be a change towards a previous implementation. Personally, I am fine with this change.

@milancurcic what was the reason of introducing this format? What do you think of this PR?

@jvdp1 jvdp1 requested review from milancurcic and a team April 25, 2024 07:25
@milancurcic
Copy link
Member

Thanks for tagging me. It was a long time ago, but I think we added the explicit edit descriptors to allow savetxt and loadtxt to do a bit-for-bit roundtrip of data. That is important for some cases, and obviously it's limiting in some others, as @chuckyvt points out. I think it's a design choice in the end, and stdlib should support a broad range of use cases. How about adding an optional argument? Not sure what's the best solution.

@perazz
Copy link
Contributor

perazz commented Apr 26, 2024

FYI I see the loadtxt test sometimes fails in the CI, see i.e. https://github.com/fortran-lang/stdlib/actions/runs/8844659963/job/24286968620?pr=801:

152/301 Test #152: loadtxt ................................***Failed    0.01 sec
At line 159 of file /Users/runner/work/stdlib/stdlib/build/src/stdlib_io.f90
Fortran runtime error: Bad value during floating point read

Error termination. Backtrace:
#0  0x104945a8e
#1  0x104946735
#2  0x10494731b
#3  0x104b66528
#4  0x104b69f19
#5  0x104b6b5cf
#6  0x104b683dd
#7  0x104513351
#8  0x104509db6
#9  0x104509df2

@jvdp1
Copy link
Member

jvdp1 commented Apr 26, 2024

How about adding an optional argument? Not sure what's the best solution.

thanks @milancurcic . I like this idea of optional argument.
What about something like:

call loadtxt (filename, array [, skiprows] [, max_rows], [, format])

similar to to_string?

The default format would be the current one.

Add format field to loadtxt function to allow user to specify the format string.  Also update loadtxt test suite.
@chuckyvt
Copy link
Contributor Author

I looked at the original pull request referenced above. It added format specifier to both write and read, which were both list directed at the time. Specifying a write format makes sense, and no changes to that in this pull request. But as far as I know a list directed read would not introduce additional rounding error or loss of precision, and is the simplest and best approach for loadtxt imho.

With that said, I updated the pull request to add an optional 'fmt' field to see how that would look and work. There currently isn't a way to specify list directed via character variable, so it has to be handled via if statements. This update uses '*' to specify list directed, but could change to anything. The thread below has some discussion on this and proposes "(LD)" for example.

https://fortran-lang.discourse.group/t/format-string-corresponding-to-list-directed-i-o/522/10

Comment on lines 149 to 157
if ( present( fmt ) ) then
if ( fmt == '*' ) then
read (s,*) d(i, :)
else
read (s,fmt) d(i, :)
endif
else
read (s,"(*"//FMT_REAL_${k1}$(1:len(FMT_REAL_${k1}$)-1)//",1x))") d(i, :)
end if
Copy link
Member

Choose a reason for hiding this comment

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

Checking the same if statement at every iteration would be quite slow. So I suggest to move this if statement outside the loop.
Could something like this work:

fmt_ = optval('*('//fmt//',1x)', '*', fmt)

do i = 1, max_rows_
  read (s, fmt_) d(i,:)
enddo

test/io/test_loadtxt.f90 Outdated Show resolved Hide resolved
test/io/test_loadtxt.f90 Outdated Show resolved Hide resolved
chuckyvt and others added 3 commits April 29, 2024 21:49
Update read loop structure.  Added optval formatting.  Additional messages added to test cases to better understand which read type failed.
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
Copy link
Member

@jvdp1 jvdp1 left a comment

Choose a reason for hiding this comment

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

LGTM. I have only some minor suggestions.

Could you also update the specs, please?

src/stdlib_io.fypp Show resolved Hide resolved
src/stdlib_io.fypp Show resolved Hide resolved
src/stdlib_io.fypp Show resolved Hide resolved
src/stdlib_io.fypp Show resolved Hide resolved
@jvdp1
Copy link
Member

jvdp1 commented May 7, 2024

Thank you @chuckyvt for the explanations. Please, before merging it, could you:

  • rebase your branch to solve the CI issue?
  • update the specs?

@chuckyvt
Copy link
Contributor Author

chuckyvt commented May 9, 2024

Ok, this should be complete. Also added a line to the loadtxt example.

Copy link
Member

@jvdp1 jvdp1 left a comment

Choose a reason for hiding this comment

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

Thank you @chuckyvt for this PR and improvement. LGTM. I will wait a couple of days, and if there are no additional comments, I will merge it.

@jvdp1
Copy link
Member

jvdp1 commented May 13, 2024

Thank you @chuckyvt . I will merge it.

@jvdp1 jvdp1 merged commit 206f84a into fortran-lang:master May 13, 2024
17 checks passed
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.

4 participants