Qucs / ADMS

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

Replace incomplete constants.vams file with new auto-generated version #101

Closed ngwood closed 2 years ago

ngwood commented 2 years ago

Replace incomplete constants.vams file with new auto-generated version.

The following table illustrates thes differences between the current and new values.

Name        Existing                New (17 s.f.)
----        --------                -------------
M_1_PI                              0.31830988618379068
M_2_PI                              0.63661977236758135
M_2_SQRTPI                          1.1283791670955126
M_E         2.71828182845904523536  2.7182818284590452
M_LN10      2.30258509299404568401  2.3025850929940457
M_LN2       0.69314718055994530941  0.69314718055994531
M_LOG10E    0.43429448190325182765  0.43429448190325182
M_LOG2E     1.44269504088896340737  1.4426950408889634
M_PI        3.14159265358979323846  3.1415926535897932
M_PI_2                              1.5707963267948966
M_PI_4                              0.78539816339744830
M_SQRT1_2                           0.70710678118654755
M_SQRT2     1.41421356237309504880  1.4142135623730950
M_TWO_PI                            6.2831853071795864

Name        Existing                New (CODATA 2018)
----        --------                -----------------
P_EPS0      8.854187817e-12         8.8541878128e-12
P_H         6.62607004081e-34       6.62607015e-34
P_K         1.3806503e-23           1.380649e-23
P_Q         1.6021766208e-19        1.602176634e-19
P_U0        1.2566370614e-6         1.25663706212e-6

Note that more significant figures can be added to the mathematical constants if desired.

Note that the existing file is missing some mathematical constants.

Note that these constants will continue to differ slightly from those defined in the Accellera standard header.

felix-salfelder commented 2 years ago

On Sun, Jan 02, 2022 at 01:39:13PM -0800, Neal wrote:

Replace incomplete constants.vams file with new auto-generated version. [..] Note that these constants will continue to differ slightly from those defined in the Accellera standard header. You can view, comment on, or merge this pull request online at:

Thanks for your work.

We are approaching the core issue -- sorry this is taking so long, but I had to refresh my memory.

How did you obtain the names of the missing constants? In my understanding, you must not copy them from the "Accellera header", because that may violate their copyright. Their work should not have to be mentioned anywhere. Please provide another reference.

For example, I could imagine that we add M_1_PI based on the fact that it is used in $some_model, and we set it to .318309 following a "wild guess". This information should be recorded as a comment.

Or... look at gnucap/include/constant.h (GPLv3+!). Perhaps we should just use it (after reformatting to verilog syntax). Some values may be outdated...

I am not a legal expert, and I may be wrong. Please consider how the current set of constants has been added and why this was necessary.

ngwood commented 2 years ago

I understand this is a sensitive issue. I'm not an IP lawyer either!

I have worked in accordance with the clean room proposal. All constant names are listed there along with a method to calculate them. That said, the M_ constant names are all POSIX defined, except for M_TWO_PI, which one can guess at being 2 times M_PI.

A scan of public CMC models shows none of them define any of the POSIX M_ constants, but together use the following.

M_LN10
M_LN2
M_PI
M_SQRT2

Thus, notably, all of the public CMC models are covered by the existing ADMS version of constants.vams. A scan of public nanoHub models show that while some models define their own local copies of the following

M_TWO_PI
M_SQRTPI
M_PI
M_LN10

outside of these model there are implicit use or REFERENCE of the following

M_1_PI
M_2_PI
M_2_SQRTPI
M_E
M_LN10
M_LN2
M_LOG10E
M_LOG2E
M_PI
M_PI_2
M_PI_4
M_SQRT1_2
M_SQRT2
M_SQRTPI
M_TWO_PI

including HOW they are mathematically defined; e.g., Colin McAndrew of NXP explicitly states what is defined in constants.vams in ALL of his affiliated models.

//  NOTE: the following constants are defined in constants.vams
//   `M_E           e
//   `M_LOG2E       log2(e)
//   `M_LOG10E      log10(e)
//   `M_LN2         ln(2)
//   `M_LN10        ln(10)
//   `M_PI          pi
//   `M_TWO_PI      pi*2.0
//   `M_PI_2        pi/2.0
//   `M_PI_4        pi/4.0
//   `M_1_PI        1.0/pi
//   `M_2_PI        2.0/pi
//   `M_2_SQRTPI    2.0/sqrt(pi)
//   `M_SQRT2       sqrt(2.0)
//   `M_SQRT1_2     1.0/sqrt(2.0)=sqrt(0.5)
//   `P_CELSIUS0    273.15          // This is a defined physical constant not subject to change
//   `P_EPS0        8.854187817e-12 // This is a defined physical constant not subject to change
//   `P_Q_NIST2010  1.602176565e-19 // Magnitude of the electronic charge (C)
//   `P_K_NIST2010  1.3806488e-23   // Boltzmann's constant (J/K)

Thus it's safe to say that the names and means of each of these constants is general knowledge.

The values I have used have all been calculated from first principles or taken from external publicly available sources without restrictions on use (NIST).

ngwood commented 2 years ago

To confirm, I did not need to use the Accellera constants.vams file in any way to create the new constants.vams file.

ngwood commented 2 years ago

I guess the alternative is to just not ship with the header files, thereby forcing users to find there own versions. I know the Xyce buildxyceplugin script, which I use, assumes ADMS has these headers though; therefore, removing them would require Xyce users who what to use this script to include a copy of the headers in their current working directory (as ADMS will look there) too.

felix-salfelder commented 2 years ago

On Mon, Jan 03, 2022 at 06:00:06AM -0800, Neal wrote:

I have worked in accordance with the clean room proposal. All constant names are listed there [..]

That's good news. Maybe we should reference (+ cite) this list, mention that all M_s (except one) are defined in POSIX, include your human readable diff, just in case, and call it a day?

Thanks

felix-salfelder commented 2 years ago

On Mon, Jan 03, 2022 at 06:22:42AM -0800, Neal wrote:

[..] use this script to include a copy of the headers in their current working directory (as ADMS will look there) too.

Wrt. "current working directory" I have openened a new ticket, #102.

ngwood commented 2 years ago

I was already made aware of the licensing issue and had been directed to the clean room list. I was also aware of 7c78bb809f1973c659aa737cc570f2dc535e7fd7 and was able to inspect the current file. I essentially implemented the clean room list as a Perl script so that is could be quickly and easily tweaked if required, without the need to manually recalculate values or risk copy/paste errors, while also being self documenting.

In isolation, adding in the missing POSIX maths constants sounds like a reasonable thing to do anyway, given the existing context. Also, there are several implicit references (use without local definition) of M_TWO_PI in publish nanoHub models, too; so it would be reasonable to assume it is a standard constant, whose value can be reasonably guessed at.

ngwood commented 2 years ago

Also, this pull request fixes issue #97.

felix-salfelder commented 2 years ago

On Mon, Jan 03, 2022 at 06:00:06AM -0800, Neal wrote:

I have worked in accordance with the clean room proposal.

I have imported the missing constants from the wiki. And I have merged your script. Does this resolve the issue?

ngwood commented 2 years ago

There are still some issues.

`define P_ESP0 8.854187817 // 10−12 F·m−1
`define P_MU0 1.2566370614 // 10−6 H / m or T·m / A or Wb / (A·m) or V·s / (A·m)

These lines should be deleted. The first overwrites the existing CORRECT value with one that is 1e12 times BIGGER; this is very broken! The second is technically not doing any harm at best, but is meaningless and could confuse some people; see #65.

If you could change

`define P_H 6.62607004081e-34 //planck constant. in \text{ J⋅s}

to

`define P_H 6.626070040e-34 //planck constant. in \text{ J⋅s}

you would also be fixing a copy/paste error.

Fixing these three things will fix everything that needs to be fixed.

felix-salfelder commented 2 years ago

On Tue, Jan 04, 2022 at 02:18:56AM -0800, Neal wrote:

There are still some issues.

`define P_ESP0 8.854187817 // 10−12 F·m−1
`define P_MU0 1.2566370614 // 10−6 H / m or T·m / A or Wb / (A·m) or V·s / (A·m)

These lines should be deleted. The first overwrites the existing CORRECT value with one that is 1e12 times BIGGER; this is very broken! The second is technically not doing any harm at best, but is meaningless and could confuse some people; see #65.

OK, these seem to be misspellings in the wiki. ESP is actually EPS and MU is actually U. The //10 should have been an E. Otherwise the values coincide with the existing ones. Deleted both lines as suggested.

If you could change define P_H 6.62607004081e-34 //planck constant. in \text{ J⋅s} to define P_H 6.626070040e-34 //planck constant. in \text{ J⋅s} you would also be fixing a copy/paste error.

I do not understand. I took the value from the qucs wiki as indicated. Both of the above do not match 6.626 070 15 x 10-34 from [1].

I'd rather put 6.62607015e-34 (?)

Thanks

[1] https://physics.nist.gov/cgi-bin/cuu/Value?h

ngwood commented 2 years ago

I missed that the first two were both typos, but it is better to delete them both.

https://en.wikipedia.org/w/index.php?title=Planck_constant&oldid=829855741 If you look at an older version of Wikipedia from the time, you will see that the value is

6.626070040(81)e−34 J⋅s = 6.626070040e−34 +/- 0.000000081e−34 J⋅s

Thus 6.626070040e−34 would have been the correct value at the time. The corrected value will then at least be consistent with the other NIST 2014 recommended values also in the header files.

ngwood commented 2 years ago

If you want them all to be the most recent NIST values and be consistent with one another (which is actually important) then you can use the 2018 set:

`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 Tue, Jan 04, 2022 at 04:48:25AM -0800, Neal wrote:

If you want them all to be the most recent NIST values and be consistent with one another (which is actually important) then you can use the 2018 set:

`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

Sounds sensible. Please commit with reference link(s?) in the commit message, or just provide the link.