Support for real32 #229

jacopo-chevallard · 2016-10-03T10:29:28Z

Hi Jacob, I'm reopening this (see #222) since I realised that maintaining a parallel fork of your original repository is quite unpractical, as it obliges me to rebase each time you submit a new release, while it would be much simpler to just integrate my modifications (if you agree!).

Nothing changed from #222, so the information contained there is still valid.

Added support for real32

…lass

zbeekman

In my very cursory overview, this all looks good to me.

In one or two places I have some minor concerns about backwards compatibility, and other than that, just code alignment issues...

My rule of thumb for code alignment issues is as follows:

If added new code blocks (multiple lines) then ensure the alignment follows the conventions in those new lines.
If a single line modification is needed, don't worry about alignment because you otherwise you'll introduce white space changes and hide any substantive change you made to the code.

Of course I defer to our BDFL, @jacobwilliams, and his all knowing judgement of all things JSON-Fortran, but wanted to add my $0.02 saying that this mostly looks great and would be a nice addition to JSON-Fortran.

zbeekman · 2016-10-03T20:23:29Z

src/json_file_module.F90

@@ -1231,7 +1331,7 @@ end subroutine wrap_json_file_update_logical
 !# See also
 !  * [[json_update_double]]

-    subroutine json_file_update_real(me,name,val,found)
+    subroutine json_file_update_double(me,name,val,found)


Was this a public method? (I'm concerned about backwards compatibility)

This is private, so it should be OK.

zbeekman · 2016-10-03T20:25:22Z

src/json_file_module.F90

@@ -1255,7 +1397,7 @@ subroutine wrap_json_file_update_real(me,name,val,found)

    class(json_file),intent(inout)       :: me
    character(kind=CDK,len=*),intent(in) :: name
-    real(RK),intent(in)                  :: val
+    real(RK32),intent(in)                  :: val


Some white space should be deleted here for stylistic consistency. I think RK was publicly exposed, but I doubt anyone was using it; the only kind parameter that should have a large impact on client code is the character kind....

zbeekman · 2016-10-03T20:27:33Z

src/json_string_utilities.F90

+        str = repeat(star,len(str))
+    end if
+
+    end subroutine real32_to_string


Is this subroutine necessary? Couldn't you just create a wrapper for the real_to_string that handles doubles and just cast the 32 bit real as a 64bit real before passing it? (You could use different default edit descriptors like you do here and just pass them in to the original to_string subroutine.)

zbeekman · 2016-10-03T20:28:00Z

src/json_value_module.F90

@@ -117,7 +117,8 @@ module json_value_module

        character(kind=CK,len=:),allocatable :: name  !! variable name

-        real(RK),allocatable                 :: dbl_value  !! real data for this variable
+        real(RK32),allocatable                 :: real_value  !! real32 data for this variable


White space alignment...

zbeekman · 2016-10-03T20:29:36Z

src/json_value_module.F90

@@ -2579,6 +2729,59 @@ subroutine wrap_json_value_add_double_vec(json, p, name, val)
    call json%add(p, to_unicode(name), val)

    end subroutine wrap_json_value_add_double_vec
+
+!*****************************************************************************************
+!> author: Jacob Williams


If you wrote this, @jacopo-chevallard, you should probably put yourself as the author... or at least add yourself

I really just did copy/paste and changed a few kinds, so no need to add my name, unless it helps to better track the work

zbeekman · 2016-10-03T20:30:11Z

src/json_value_module.F90

+
+    class(json_core),intent(inout)      :: json
+    character(kind=CK,len=*),intent(in) :: str
+    real(RK32)                            :: rval


zbeekman · 2016-10-03T20:30:25Z

src/json_value_module.F90

+
+    class(json_core),intent(inout) :: json
+    type(json_value),pointer       :: me
+    real(RK32),intent(out)           :: value


zbeekman · 2016-10-03T20:30:53Z

src/json_value_module.F90

+    class(json_core),intent(inout)      :: json
+    type(json_value),pointer            :: p
+    character(kind=CK,len=*),intent(in) :: name
+    real(RK32),intent(in)                 :: val


zbeekman · 2016-10-03T20:31:11Z

src/json_value_module.F90

+    character(kind=CDK,len=*),intent(in)          :: path
+    real(RK32),dimension(:),allocatable,intent(out) :: vec
+    logical(LK),intent(out),optional              :: found
+


zbeekman · 2016-10-03T20:37:18Z

Also my other concern is minimizing code duplication... I forgot to mention that in my review above.

jacobwilliams · 2016-10-04T00:30:38Z

src/json_file_module.F90

+!>
+!  Get a pointer of the [[json_value]] in this [[json_file]].
+
+    subroutine get_json_value_in_file(me,value)


This routine is not necessary, since it does the same thing as json_file_get_root, unless I'm missing something.

I guess you're right, calling the generic get with a json_value argument will do the job

jacobwilliams · 2016-10-05T17:59:12Z

Give me some time to look at this. I'm OK in principle with merging it in. Thanks!

jacopo-chevallard · 2016-10-06T07:04:09Z

Ok, good!

jacobwilliams · 2017-08-24T20:59:00Z

I guess I lost track of this. Are the recent changes where you can specify the kinds of the integer and reals via a compiler directive enough for your purposes?

I'm now hesitant about this because if we add real32's why not also real128's? Or all the various integer kinds? The way it's structured now, that would be a ton of duplicate code and wrappers to add (unless and until we can get some kind of sane generic programming feature in Fortran 2020...)

jacopo-chevallard · 2017-08-25T07:59:20Z

hi Jacob,

Are the recent changes where you can specify the kinds of the integer and reals via a compiler directive enough for your purposes?

I fear not, since I need to be able to use both real kind at the same time

I'm now hesitant about this because if we add real32's why not also real128's? Or all the various integer kinds?

I think it's safe to say that in 99% of the scientific codes the only used real kinds are "single" (real32) and double (real64) precision, so there is no need to worry about real128, ...
Similarly, most scientific codes perform calculations with real and not integer numbers, so I don't think there will be any strong need of different integer kinds.

The way it's structured now, that would be a ton of duplicate code and wrappers to add (unless and until we can get some kind of sane generic programming feature in Fortran 2020...)

Also, I agree that generic programming (templating-like) would ease a lot these types of implementations, but you can already get a templating-like programming using modern Python-based pre-processors like fypp.
This is what I do usually, and it helped me a lot in reducing errors and to ease the update of the code (I only have to update one function/subroutine, then fypp will create the different combinations of types/shapes).
You can take a look at the attached archived to get an idea of how to sue fypp for templating.

fypp_example.tar.gz

fypp_example2.tar.gz

I created the *f90 files from the *fpp ones with command

 fypp file_name.fpp file_name.f90 -l 100 -f smart

jacobwilliams · 2019-01-21T21:23:13Z

I'm going to assume this has been superseded by: #239 and #387.

jacopo-chevallard-test added 14 commits July 19, 2016 15:22

Added tyoe for 4-byte real

12a3bd4

Added JSON code for real32 (4-byte real) type

d629a0f

Added real32 to string conversion

fae3698

Added JSON code for real32

bb8bdc2

First attempt to modify the code to supprt usage of 4-byte real (RK32)

edb7979

First attempt to modify the code to supprt usage of 4-byte real (RK32)

cad5bb4

Corrected bugs dbl_value -> real_value

359639d

Added default formatting for real32

8fc2274

Usage of default formatting for real32

3b5a926

Added unit tests for real32 routines

f26522c

Merge branch 'hotfix/5.0.3'

27f087a

Merge tag '5.0.3' into develop

d754843

Added support for real32

Added function to extract json_value pointer from current json_file c…

9296fe4

…lass

Merge branch 'release/5.0.3b'

b91608d

zbeekman reviewed Oct 3, 2016

View reviewed changes

jacobwilliams reviewed Oct 4, 2016

View reviewed changes

jacobwilliams mentioned this pull request Nov 18, 2016

Error when reading real array value #236

Closed

jacobwilliams mentioned this pull request Jan 21, 2019

Support for multiple real kinds #386

Closed

jacobwilliams closed this Jan 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for real32 #229

Support for real32 #229

jacopo-chevallard commented Oct 3, 2016

zbeekman left a comment

zbeekman Oct 3, 2016

jacobwilliams Oct 4, 2016

zbeekman Oct 3, 2016

zbeekman Oct 3, 2016

zbeekman Oct 3, 2016

zbeekman Oct 3, 2016

jacopo-chevallard Oct 4, 2016

zbeekman Oct 3, 2016

zbeekman Oct 3, 2016

zbeekman Oct 3, 2016

zbeekman Oct 3, 2016

zbeekman commented Oct 3, 2016

jacobwilliams Oct 4, 2016

jacopo-chevallard Oct 4, 2016

jacobwilliams commented Oct 5, 2016

jacopo-chevallard commented Oct 6, 2016

jacobwilliams commented Aug 24, 2017

jacopo-chevallard commented Aug 25, 2017

jacobwilliams commented Jan 21, 2019

Support for real32 #229

Support for real32 #229

Conversation

jacopo-chevallard commented Oct 3, 2016

zbeekman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbeekman commented Oct 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobwilliams commented Oct 5, 2016

jacopo-chevallard commented Oct 6, 2016

jacobwilliams commented Aug 24, 2017

jacopo-chevallard commented Aug 25, 2017

jacobwilliams commented Jan 21, 2019