Skip to content

Savetxt unit#1085

Open
fiolj wants to merge 49 commits intofortran-lang:masterfrom
fiolj:savetxt-unit
Open

Savetxt unit#1085
fiolj wants to merge 49 commits intofortran-lang:masterfrom
fiolj:savetxt-unit

Conversation

@fiolj
Copy link
Copy Markdown
Contributor

@fiolj fiolj commented Jan 5, 2026

This PR aims to add optional arguments to savetxt, that behave similar to numpy's savetxt.

This is associated with Issue 263 and this discussion thread.

It add the possibility of supplying the unit of an open file instead of a filename (which could be used for output_unit for instance)

This implementation is quite simple. The main changes are:

  1. Add optional header and footer (that could be commented out with a character, currently defaults to '#')
  2. Changes delimiter to an arbitrary length string. I am not completely sure that is really useful. Initially I've added this because np.savetxt (Numpy's) allows character or arbitrary strings but np.loadtxt requires it to be a length 1 char.
  3. Add the possibility to override default format for saving data

@codecov
Copy link
Copy Markdown

codecov bot commented Jan 5, 2026

Codecov Report

❌ Patch coverage is 83.95062% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.80%. Comparing base (fb63d7e) to head (03cd77c).
⚠️ Report is 27 commits behind head on master.

Files with missing lines Patch % Lines
example/io/example_savetxt.f90 0.00% 13 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1085      +/-   ##
==========================================
+ Coverage   68.00%   68.80%   +0.79%     
==========================================
  Files         404      407       +3     
  Lines       12935    13690     +755     
  Branches     1392     1543     +151     
==========================================
+ Hits         8797     9419     +622     
- Misses       4138     4271     +133     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jalvesz
Copy link
Copy Markdown
Contributor

jalvesz commented Jan 6, 2026

Thanks for this @fiolj. Would you mind reverting the changes not related to the implementation? (styling) There are too many and it renders difficult to read through the PR. You can check the style_guide for info https://github.com/fortran-lang/stdlib/blob/master/STYLE_GUIDE.md

One thing, white spaces in-between parentheses and an intrinsic function are not recommended ( open (...), allocate (...), etc )

@jalvesz jalvesz linked an issue Jan 6, 2026 that may be closed by this pull request
@fiolj
Copy link
Copy Markdown
Contributor Author

fiolj commented Jan 6, 2026

Thanks @jalvesz, I've fixed the formatting

Copy link
Copy Markdown
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 @fiolj . Here are some suggestions

@fiolj
Copy link
Copy Markdown
Contributor Author

fiolj commented Jan 15, 2026

Reviewing the arguments of savetxt I've rechecked and Numpy's has the following order:
np.savetxt(fname, X, fmt, delimiter, newline, header, footer, comments, encoding)

Currently, we have the order of fmt and delimiter inverted. Should we make it as Numpy or should we keep it?

Copy link
Copy Markdown
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. Thank you @fiolj

Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
@fiolj
Copy link
Copy Markdown
Contributor Author

fiolj commented Feb 11, 2026

In my opinion it has converged, I would be happy to make changes if necessary. Could we get a new review and try to complete the task and merge this PR?

Copy link
Copy Markdown
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 @fiolj . Overall LGTM. There are still a few open comments/suggestions. Could you review them and implement/close them, please?

#:else
inquire (unit=unit, opened=opened)
if(.not. opened) then
write (msgout,'(a,i0,a)') 'savetxt error: unit ',unit,' not open'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@fiolj Is this a valid suggestion?

Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
@fiolj
Copy link
Copy Markdown
Contributor Author

fiolj commented Feb 12, 2026

Thanks @jvdp1 for the review.
With regard to this comment, I do not understand well the question.

  • I've simplified the error handling when something happens at reading the values (message formatted with format 1) by using a string variable fout.
  • I've now also added a call to error_stop() when unit is not open (line 332).
  • Also I've cleaned-up some comments

@fiolj fiolj closed this Feb 12, 2026
@fiolj fiolj reopened this Feb 12, 2026
fiolj and others added 3 commits February 11, 2026 23:28
Added:
- comments with intent of variable `fout`
- stop the program if unit file is not open
- clean-up comments
Added clarification of use with filename and unit.
Also added an example
@fiolj
Copy link
Copy Markdown
Contributor Author

fiolj commented Feb 13, 2026

I've updated the specs to clarify the behavior when the file already exists (as suggested in the discussion thread):

  • When used with filename as a first argument it will overwrite the file
  • When used with unit as first argument it will append data

@fiolj
Copy link
Copy Markdown
Contributor Author

fiolj commented Feb 21, 2026

I've updated the specs to clarify the behavior when the file already exists (as suggested in the discussion thread):

* When used with `filename` as a first argument it will overwrite the file

* When used with `unit` as first argument it will append data

I've been thinking on the suggestion on the the discussion thread) about consistency of the arguments. The use of an additional argument append would give a consistent behavior.
The spec would change to something like:


Syntax

call savetxt(filename, array [, delimiter] [, fmt] [, header] [, footer] [, comments] [, append])

call savetxt(unit, array[, delimiter] [, fmt] [, header] [, footer] [, comments] [, append])

Arguments

...

append (optional): Shall be a logical flag. If .true. data will be appended at the end of the file or unit. If .false. file will be overwritten. Default: .false.


The only problem that I can see is that when used with an unit number, the function will always be modifying the position of the file (to the beginning or the end). In the previous version the user could in principle position it arbitrarily.

Thoughts, preferences?

@fiolj
Copy link
Copy Markdown
Contributor Author

fiolj commented Mar 27, 2026

Hi, I've reverted some of the changes, to the previous version, removing the append option for filenames.
Also, added some checks to the file and unit (open, writable)
I think that there are not previous open comments or suggestions pending. If contributors agree, I'd like it to be merged

Copy link
Copy Markdown
Contributor

@jalvesz jalvesz left a comment

Choose a reason for hiding this comment

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

LGTM @fiolj! modulo some cosmetic suggestions I think this is ready to be merged.

fiolj and others added 3 commits April 2, 2026 16:09
Co-authored-by: José Alves <102541118+jalvesz@users.noreply.github.com>
Co-authored-by: José Alves <102541118+jalvesz@users.noreply.github.com>
Co-authored-by: José Alves <102541118+jalvesz@users.noreply.github.com>
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.

Add optional arguments to savetxt

4 participants