Qucs / ADMS

ADMS is a code generator for the Verilog-AMS language
GNU General Public License v3.0
94 stars 31 forks source link

Issues with ADMS' constants.vams #97

Closed ngwood closed 2 years ago

ngwood commented 3 years ago

Firstly, and less importantly, the CODATA 2014 recommended SI value of Planck constant is 6.626070040e-34 with standard error of 8.1e-42. Whoever defined P_H in constants.vams wasn't aware of the notation used to denote this standard error and has instead unintentionally defined its value as 6.62607004081e-34. It might be worth someone adding a comment to highlight this fact.

Secondly, and more importantly, the current Accellera standard requires use of CODATA 1998 recommended SI values by default, for backwards compatibility; however, options are made available to explicitly select between SPICE, legacy and CODATA 2010 values. Is there a reason we are providing a non-compliant version of constants.vams? Do we even need to provide this file at all? Should we provide a compliant version?

tvrusso commented 3 years ago

See commit 9725a23 and issue #35 for why a QUCS-specific constants.vams is provided.

ngwood commented 3 years ago

Thank you. It didn't strike me that the latter issue was a conscious decision.

felix-salfelder commented 3 years ago

Thanks for raising this issue.

CODATA 2014 [..] Planck constant is [..] constants.vams wasn't aware [..] comment

It is safe to assume that constants.vams does not follow recommendations and does not meet requirements. This is consistent with the adms package, which has no guarantees or liabilities attached.

The value listed for P_H has been taken from Wikipedia (see the enclosed comment). If there is a good reason to truncate it, we could do so.

Please send a patch adding comments if more clarity is needed.

Secondly, and more importantly, the current Accellera standard [..] use of CODATA 1998 recommended SI [..] explicitly select [..]

If it is practical to follow a standard, we could try. What if the standard changes? What if it becomes obsolete?

There is an -I flag that influences the `include directive. I believe it may help to explicitly select various versions of the numerical values in question.

Do we even need to provide this file at all?

No. It is probably best to use whatever is needed, mainly in terms of compatibility. We do provide one for historical reasons, perhaps for convenience. Eventually, some applications depend on its presence.

On Sun, Jul 25, 2021 at 09:08:08AM -0700, Tom Russo wrote:

See commit 9725a23 and issue #35 for why a QUCS-specific constants.vams is provided.

The supplied constants.vams is not particularly QUCS specific, nor intentionally so.

tvrusso commented 3 years ago

On Sun, Jul 25, 2021 at 09:08:08AM -0700, Tom Russo wrote: See commit 9725a23 and issue #35 for why a QUCS-specific constants.vams is provided. The supplied constants.vams is not particularly QUCS specific, nor intentionally so.

I only meant that it was a file that was created for QUCS to replace the "standard" Accellera file due to licensing restrictions, as stated in the issue and commit I linked to.

felix-salfelder commented 3 years ago

On Mon, Jul 26, 2021 at 08:40:35AM -0700, Tom Russo wrote:

The supplied constants.vams is not particularly QUCS specific, nor intentionally so.

I only meant that it was a file that was created for QUCS to replace the "standard" Accellera file due to licensing restrictions, as stated in the issue and commit I linked to.

Yes and no, and I understand why it appears to be as you say.

Replacing the files was necessary in order to distribute adms freely, an issue that emerged in 2008.

Guilherme was a/the QUCS maintainer in 2016, and kindly put it together, after I nudged him. I hardly knew what QUCS was, so it can't have been entirely for the purpose of QUCS.

tvrusso commented 3 years ago

I should not have said "QUCS" specific. I meant "ADMS" specific. This is what I get for responding with half a brain to an issue in QUCS/ADMS.

ngwood commented 3 years ago

Thank you both for your insights.

My reason for bringing up issue 1 is because I'm extremely pedantic. Could the discrepancy in P_H alone affect single precision accuracy of simulations? Yes. Is it a big deal? No. The various recommended values from CODATA, which is what ends up on Wikipedia, are considered the most reliable values at the time of release, but as you mention they are potentially subject to change. The current Accelera standard, which dates back to 2014, provides only the 1998 (default) and 2010 (PHYSICAL_CONSTANTS_NIST2010) recommended values. Note that if they had changed the defaults from the 1998 values then the standard would not be backwards compatible. Also, they are not going to release a new standard just to update the physical constants every 4 years. As a side note, the Accelera standard doesn’t even define the correct values for the PHYSICAL_CONSTANTS_SPICE build option, so I wouldn’t worry too much. The value of Wikipedia at the time was in fact correct, but my guess is that someone misunderstood the notation used in denoting the value and its uncertainty. All that being said, due to a fairly recent redefinition of some of the physical constants, specifically for use in redefining SI units, the currently published CODATA recommended value of P_H (6.62607015e-34) is actually considered "exact” and is very unlikely to be altered, at least not any time soon. The same goes for P_Q (1.602176634e-19) and P_K (1.380649e-23), although notably not for P_EPS0 (8.8541878128e-12). I don’t think any of the CMC standard models use P_H, although I might be wrong on that. Some actually locally define versions of P_Q and P_K, too, but not all do, importantly. If you’re not worried about following the standard, I’d say either keep things as they are (perhaps with a few comments for pedantic people such as myself), revert to the pre ADMS v2.3.5 (i.e., CODATA 1998 as per the Accelera standard) values, or update the values to use the stable CODATA 2018 values; the first option would be the least intrusive for most active ADMS users, the second would better match the Accelera standard, while the last would be the idealistic solution.

My reason for bringing up issue 2 is because I noticed that Xyce appears to use ADMS’ constants.vams and on inspection of the file I noticed that two of the physical constants in ADMS were changed in the 2.3.5 release and I was wondering why that was given that it breaks the standard. I see now that is not a major concern.

FYI, there are also a few missing standard maths constants as well I’ve noticed.

felix-salfelder commented 3 years ago

On Mon, Jul 26, 2021 at 12:09:35PM -0700, ngwood wrote:

[..] I noticed that two of the physical constants in ADMS were changed [..]. I see now that is not a major concern.

The relevant change was the license, not the values. For a reason that is surfacing in this discussion, we provide header files that allow modification and redistribution. You are advised to study the license and include a copy with your (free/libre) software and modify it to fit your needs.

Quoting from the file in question, for the record. """ This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more details. """

One feasible approach to adms could be to add as many digits as possible. Simply following science, and ignoring standards. I believe this is feasible, because it allows a maintainer to decide in finite time which changes to apply and why. Please speak up if you (are a maintainer and) disagree.

FYI, there are also a few missing standard maths constants as well I’ve noticed.

Please, could you provide the identifiers and values? I'd be happy to add them. This ticket should be closed, after a reasonable amount of missing constants have been added. Thanks.

ngwood commented 3 years ago

I've edited the existing ADMS header file. In the new configuration the standard values would be used by default, making the file nominally compliant, while the recommended modern SI-standardised values, taken from here, would be selectable via a non-standard -IPHYSICAL_CONSTANTS_2018_CODATA command line option to admsXml, as an example.

Please see attached attached.

ngwood commented 2 years ago

I created a Perl script to calculate all of the relevant mathematical constants and extract the relevant physical constants from NIST. The accuracy for all mathematical constants has been set to 17; i.e., minimum to guarantee double precision. All physical constants inherit the description and units from NIST. ADMS banner is also output.

mkconstants.pl.txt

bash> wget https://physics.nist.gov/cuu/Constants/Table/allascii.txt
--2022-01-02 10:49:50--  https://physics.nist.gov/cuu/Constants/Table/allascii.txt
SSL_INIT
Resolving physics.nist.gov (physics.nist.gov)... 129.6.13.138
Connecting to physics.nist.gov (physics.nist.gov)|129.6.13.138|:443... connected.
HTTP request sent, awaiting response... 200 OK
Length: 40689 (40K) [text/plain]
Saving to: ‘allascii.txt’

allascii.txt        100%[===================>]  39.74K  --.-KB/s    in 0.1s    

2022-01-02 10:49:51 (321 KB/s) - ‘allascii.txt’ saved [40689/40689]

bash> chmod u+x mkconstants.pl 
bash> ./mkconstants.pl 

This produces the following.

/*
Copyright (C) 2016 Guilherme Brondani Torri <guitorri@gmail.com>
              2016 Felix Salfelder <felix@salfelder.org>
              2022 Neal Graham Wood <neal.wood@protonmail.com>

This program is free software: you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
the Free Software Foundation, either version 3 of the License, or
(at your option) any later version.

This program is distributed in the hope that it will be useful,
but WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
GNU General Public License for more details.

You should have received a copy of the GNU General Public License
along with this program.  If not, see <http://www.gnu.org/licenses/>.
*/

`ifdef CONSTANTS_VAMS
`else
`define CONSTANTS_VAMS 1

/* Mathematical constants */

`define M_1_PI                  0.31830988618379068
`define M_2_PI                  0.63661977236758135
`define M_2_SQRTPI              1.1283791670955126
`define M_E                     2.7182818284590452
`define M_LN10                  2.3025850929940457
`define M_LN2                   0.69314718055994531
`define M_LOG10E                0.43429448190325182
`define M_LOG2E                 1.4426950408889634
`define M_PI                    3.1415926535897932
`define M_PI_2                  1.5707963267948966
`define M_PI_4                  0.78539816339744830
`define M_SQRT1_2               0.70710678118654752
`define M_SQRT2                 1.4142135623730950
`define M_TWO_PI                6.2831853071795864

/* Physical constants */

// speed of light in vacuum -- m s^-1
`define P_C                     299792458

// zero degrees Celsius -- K
`define P_CELSIUS0              273.15

// vacuum electric permittivity -- F m^-1
`define P_EPS0                  8.8541878128e-12

// Planck constant -- J Hz^-1
`define P_H                     6.62607015e-34

// Boltzmann constant -- J K^-1
`define P_K                     1.380649e-23

// elementary charge -- C
`define P_Q                     1.602176634e-19

// vacuum mag. permeability -- N A^-2
`define P_U0                    1.25663706212e-6

`endif // CONSTANTS_VAMS

The sytle and formatting can be changed by tweaking a few numbers.

Would this constants.vams be suitable for use with ADMS?

felix-salfelder commented 2 years ago

What's the diff between the current and proposed constants header? I remember one last time it was too long, and I gave up (sorry).

How about this:

  1. patch constants as/where needed (minimal diff)
  2. put the perl script into contrib (why not?)
  3. release

Thanks.

ngwood commented 2 years ago

This should illustrate the difference in terms of what is actually defined by each. Here orig is the current ADMS file and new is the generated one. The resolution of mathematical constants for new is not so arbitrarily set to 17 significant places. The physical constants for new are the most recent recommended values, where as for orig they are a mixture of previous recommended values. Also orig is missing many things, but new has everything orig has.

bash> for f in constants.vams.{orig,new}; do grep "define [MP]_" ${f} | tr -s ' ' | cut -f 1,2,3 -d ' ' | sort > ${f}.grep; done
bash> diff constants.vams.{orig,new}.grep
1,7c1,14
< `define M_E 2.71828182845904523536
< `define M_LN10 2.30258509299404568401
< `define M_LN2 .69314718055994530941
< `define M_LOG10E .43429448190325182765
< `define M_LOG2E 1.44269504088896340737
< `define M_PI 3.14159265358979323846
< `define M_SQRT2 1.41421356237309504880
---
> `define M_1_PI 0.31830988618379068
> `define M_2_PI 0.63661977236758135
> `define M_2_SQRTPI 1.1283791670955126
> `define M_E 2.7182818284590452
> `define M_LN10 2.3025850929940457
> `define M_LN2 0.69314718055994531
> `define M_LOG10E 0.43429448190325182
> `define M_LOG2E 1.4426950408889634
> `define M_PI 3.1415926535897932
> `define M_PI_2 1.5707963267948966
> `define M_PI_4 0.78539816339744830
> `define M_SQRT1_2 0.70710678118654752
> `define M_SQRT2 1.4142135623730950
> `define M_TWO_PI 6.2831853071795864
10,14c17,21
< `define P_EPS0 8.854187817e-12
< `define P_H 6.62607004081e-34
< `define P_K 1.3806503e-23
< `define P_Q 1.6021766208e-19
< `define P_U0 1.2566370614e-6
---
> `define P_EPS0 8.8541878128e-12
> `define P_H 6.62607015e-34
> `define P_K 1.380649e-23
> `define P_Q 1.602176634e-19
> `define P_U0 1.25663706212e-6
felix-salfelder commented 2 years ago

On Sun, Jan 02, 2022 at 04:30:55AM -0800, Neal wrote:

bash> for f in constants.vams.{orig,new}; do grep "define [MP]_" ${f} | tr -s ' ' | cut -f 1,2,3 -d ' ' | sort > ${f}.grep; done bash> diff constants.vams.{orig,new}.grep 1,7c1,14 < `define M_E 2.71828182845904523536 [..]

Alright, Let's reformat constants. Please put the above into your commit message.

ngwood commented 2 years ago

For me it's largely about getting the missing constants into ADMS.

bash> for f in constants.vams.{orig,new}; do grep "define M_" ${f} | cut -f 2 -d ' ' | sort > ${f}.grep; done
bash> diff constants.vams.{orig,new}.grep
0a1,3
> M_1_PI
> M_2_PI
> M_2_SQRTPI
6a10,12
> M_PI_2
> M_PI_4
> M_SQRT1_2
7a14
> M_TWO_PI

I'm happy to do just that. I suspect changing values might actually have a knock on effect for some people. That said, the constants already changed markedly once already, in v2.3.5 and no one seemed to notice. Also, the physical constants don't match the Accellera standard values either.

The idea behind the script was to have some level of flexibility, self documentation and to reduce the chance of human error. I can certainly add back in some digits to the mathematical constants to make the diff less striking.

If I were to add the script to contrib how should I organise it?

felix-salfelder commented 2 years ago

On Sun, Jan 02, 2022 at 09:20:49AM -0800, Neal wrote:

For me it's largely about getting the missing constants into ADMS. bash> for f in constants.vams.{orig,new}; do grep "define M_" ${f} | cut -f 2 -d ' ' | sort > ${f}.grep; done [..] I'm happy to do just that. I suspect changing values might actually have a knock on effect for some people.

Go for it. With a human-readable (git) diff, some people can diagnose or speculate about the effects. I will only merge it.

That said, the constants already changed markedly once already, in v2.3.5 and no one seemed to notice.

The file was not changed, it had to be replaced. I never tried to compare the numbers or the file contents.

If I were to add the script to contrib how should I organise it?

Up to you. I'm fine if you just put it there.

ngwood commented 2 years ago

This issue was fixed by commit 7653177. Thank you! I can now use ADMS with my models without the need for an external header file.