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

[stdlib_io] add disp(display variable values formatted). #520

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

zoziha
Copy link
Contributor

@zoziha zoziha commented Sep 11, 2021

  • add disp.

Note: Because disp is a very important IO routine, I hope it will be discussed and approved more widely before it is adopted.

Description

Print your data to a output unit with a header. (more detail)

call [[stdlib_io(module):disp(interface)]]  &
( [x , header, unit=output_unit, brief=.false., format="g0.4", width=80, sep="  " ] )

Example:

use stdlib_strings, only: to_string
use stdlib_io,      only: disp, open

call disp("It is a note.")
call disp(x, "I am a scalar:")

i = open("array.txt")
call disp(A(1:50, 2:4), "Let me see the array:", unit=i, brief=.true.)
do j = 1, 3
    call disp(B(2:10,:,j), "Array B, index: "//to_string(j), unit=i)
end do
close(i)

Prior Art

Notes

disp is the first routine I submitted to stdlib, because the original PR discussion became very long and not suitable for review (see #445).
And I resubmitted this PR now.

@zoziha zoziha mentioned this pull request Sep 11, 2021
7 tasks
@awvwgk awvwgk linked an issue Sep 18, 2021 that may be closed by this pull request
@awvwgk awvwgk added reviewers needed This patch requires extra eyes topic: IO Common input/output related features labels Sep 18, 2021
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 for this PR. Here are some comments on the specs.

doc/specs/stdlib_io.md Outdated Show resolved Hide resolved

### Arguments

`x`: Shall be a `logical/integer/real/complex/string_type` scalar or `logical/integer/real/complex` and rank-1/rank-2 array.
Copy link
Member

Choose a reason for hiding this comment

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

Why not array of characters or of string_type?

Copy link
Contributor Author

@zoziha zoziha Oct 3, 2021

Choose a reason for hiding this comment

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

The character length of array of string_type is often uncertain, and it is difficult to print them uniformly.
And we already have stringlist, its internal elements are private, and the outside is inaccessible.
(see stdlib_stringlist_type.f90#L52~L54)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Support for string_type type is added: string_type scalar, rank-1/rank-2 array.
Only character(*) scalar is supported, string_type array should be more easy to use than character(*) array.
:)

doc/specs/stdlib_io.md Outdated Show resolved Hide resolved
@zoziha zoziha marked this pull request as draft November 8, 2021 01:31
@zoziha zoziha changed the title [stdlib_io] add disp(display your data). [stdlib_io] add disp(display variable values formatted). Nov 9, 2021
@zoziha zoziha marked this pull request as ready for review November 9, 2021 14:21
@zoziha
Copy link
Contributor Author

zoziha commented Nov 9, 2021

I redesigned and updated the internal implementation of disp, it supports more control arguments, and now also supports the corresponding array of string_type type.
Therefore, poor internal design may be introduced. Please suggest amendments to make the disp more formal. 😊

Questions

  • width=80 by default.
    Considering that disp has the need to output to the screen, 80 is a commonly used size.
  • Whether the outputted characters are left-justified (now solution) or right-justified (Pretty printing of matrices (and multidimensional arrays) #40 (comment))?
    It seems that the left alignment needs to be neater.
  • Is the parameter sep required? How to determine the name sep?
    sep(separator).
  • The compilation volume of the stdlib_io_disp.o submodule under gfortran has reached 800KB.
    I have tried my best to improve it. If there is a way to improve the compilation volume, please let me know and I will try it.

Thank you :)

brief_ = optval(brief, .false.)
format_ = optval(format, "g0.4")

#! width have to be greater than or equal 80 by default
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
#! width have to be greater than or equal 80 by default
#! width have to be greater than or equal to 80 by default

@zoziha zoziha marked this pull request as draft November 10, 2021 05:56
@zoziha zoziha marked this pull request as ready for review November 10, 2021 05:59
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 for this proposition. Here are some comments.

Regarding the tests, I guess that they should be moved to support the test-drive features, introduced recently in stdlib. However, since this PR is open since a while, I would understand that you would prefer to not do it.

- `x`: Shall be a `logical/integer/real/complex/character(len=*)/string_type` scalar or `logical/integer/real/complex/string_type` and rank-1/rank-2 array.
This argument is `intent(in)` and `optional`.

- `header`: Shall be a `character(len=*)` scalar.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `header`: Shall be a `character(len=*)` scalar.
- `header`: Shall be a `character` scalar.

This argument is `intent(in)` and `optional`.

- `unit`: Shall be an `integer` scalar, linked to an IO stream.
This argument is `intent(in)` and `optional`.<br>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This argument is `intent(in)` and `optional`.<br>
This argument is `intent(in)` and `optional`.

The default value is `output_unit` from `iso_fortran_env` module.

- `brief`: Shall be a `logical` scalar, controls an abridged version of the `x` array to be outputted.
This argument is `intent(in)` and `optional`.<br>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This argument is `intent(in)` and `optional`.<br>
This argument is `intent(in)` and `optional`.

This argument is `intent(in)` and `optional`.<br>
The default value is `.false.`

- `format`: Shall be a `character(len=*)` scalar.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `format`: Shall be a `character(len=*)` scalar.
- `format`: Shall be a `character` scalar.

The default value is `.false.`

- `format`: Shall be a `character(len=*)` scalar.
This argument is `intent(in)` and `optional`.<br>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This argument is `intent(in)` and `optional`.<br>
This argument is `intent(in)` and `optional`.


- `sep`: Shall be a `character(len=*)` scalar, separator.
This argument is `intent(in)` and `optional`.<br>
The default value is "&ensp;&ensp;", two spaces.
Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by two spaces?


### Output

The result is to print `header` and `x` on the screen (or another output `unit/file`) in this order.<br>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The result is to print `header` and `x` on the screen (or another output `unit/file`) in this order.<br>
The result is to print `header` and `x` on the screen (or another output `unit/file`) in this order.


### Example

```fortran
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to reduce the example. One or two disp should be enough for the reader to understand its aim.

!> version: experimental
!>
!> Display a scalar, vector or matrix formatted.
!> ([Specification](../page/specs/stdlib_io.html#display-the-value-of-the-variable))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
!> ([Specification](../page/specs/stdlib_io.html#display-the-value-of-the-variable))
!> ([Specification](../page/specs/stdlib_io.html#disp-display-the-value-of-the-variable))

width_ = optval(width, 80)
width_ = merge(width_, 80, width_ > 80)

sep_ = optval(sep, " ")
Copy link
Member

Choose a reason for hiding this comment

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

Why "two spaces" instead of "one space"?

@jvdp1 jvdp1 added the waiting for OP This patch requires action from the OP label Dec 8, 2021
@zoziha zoziha marked this pull request as draft December 14, 2021 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reviewers needed This patch requires extra eyes topic: IO Common input/output related features waiting for OP This patch requires action from the OP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pretty printing of matrices (and multidimensional arrays)
3 participants