Add python3 backend. #1

dvc94ch · 2018-01-20T14:20:29Z

No description provided.

kasbah · 2018-01-20T14:31:08Z

Looks good to me, are you planning to do more work on this?

The way I was going to approach this was to start with JS and develop the grammar against the old tests and then work on the other languages.

kasbah · 2018-01-20T14:33:03Z

Makefile

@@ -12,7 +12,7 @@ clean-java:
 	rm -f java/*

 python3: $(GRAMMAR) $(GRAMMAR_FILES)
-	$(ANTLR) -Dlanguage=Python3 $< -o python3/
+	$(ANTLR) -Dlanguage=Python3 $< -o python3/antlr

 clean-python3:
 	rm -f python3/*


You probably want to change the clean directory too.

kasbah · 2018-01-20T15:17:58Z

python3/electro_grammar.py

+    def enterCapacitance(self, ctx):
+        cprefix_lookup = {'u': 10e-6, 'n': 10e-9, 'p': 10e-12}
+        number = float(str(ctx.NUMBER()))
+        cprefix = str(ctx.CPREFIX())


I think you forgot to use the cprefix_lookup. I kind of want to make the conversion and normalisation part of the grammar but that may be awkward or impossible.

Also, maybe we should be using decimal for any values as I observed floating point errors in my previous python port.

dvc94ch · 2018-01-20T17:50:33Z

js/lib/index.js

+	var parser = new ElectroGrammarParser(tokens);
+	parser.buildParseTrees = true;
+
+	try {


Completely useless, haven't figured out how to detect errors yet

I believe we need to create an ErrorListener. Do you have a copy of the Antlr4 book?

dvc94ch · 2018-01-20T17:58:50Z

Current goal is feature parity to previous electro grammar version in js and python backends. I'm thinking about adding a yosys-json frontend where the 'electro-grammar' attribute is parsed and a 'cpl' attribute is added. This would make a python version unnecessary for pycircuit. Do you think it's in the scope of electro-grammar or belongs into pycircuit?

kasbah · 2018-01-20T18:03:26Z

.gitignore

 coverage/
 .nyc_output/
 node_modules/
 test-lib/
+
+**/ElectroGrammar*


Won't this ignore ElectorGrammar.g4?

kasbah · 2018-01-20T18:04:00Z

.gitignore

@@ -1,4 +1,7 @@
+package-lock.json


We should commit the lock.

aren't locks just for applications? this is a library, so the versions should match what the end user uses.

kasbah · 2018-01-20T18:07:31Z

python3/.gitignore

+MANIFEST
+.cache/
+*.pyc
+Pipfile.lock


Again, we can commit the locks.

kasbah · 2018-01-20T18:09:40Z

js/test/test_parse.js

-})
+      assert(parse('0603').component.type === 'unknown');
+    });
+});


If you are having issues with the formatting then we could just apply prettier to all the JS code.

The index.js file uses 2 spaces indentation and has semicolons at the end. Was updating it to match. What's your preference?

I added the prettier code formatter, you can apply a uniform style to everything in js/* by doing npm r fmt.

kasbah · 2018-01-20T18:10:50Z

package.json

+      "transform-object-assign"
+    ]
+  }
+}


We already have a package.json for the js bit. I guess you want one for the CPL script, it doesn't need all these deps then though.

kasbah · 2018-01-20T18:11:29Z

prepare_cpl.js

@@ -1,6 +1,6 @@
 const yaml  = require('js-yaml')
 const fs    = require('fs')
-const parse = require('./src/parse')
+const parse = require('./js/lib/index.js').parse


You can do

const {parse} = require('./js/lib')

kasbah · 2018-01-20T18:12:39Z

src/ElectroGrammar.g4


-capacitance : NUMBER CPREFIX FARAD?;
+electro_grammar : capacitor;


This is fine for now but ultimately I want the parser to work on individual specs like 50V and 10% without outputting a complete component.

To be clear: it's fine to just port the existing functionality for now. Just something to keep in mind for the future.

Yes, that makes sense

kasbah · 2018-01-20T18:25:11Z

I'm thinking about adding a yosys-json frontend where the 'electro-grammar' attribute is parsed and a 'cpl' attribute is added. This would make a python version unnecessary for pycircuit. Do you think it's in the scope of electro-grammar or belongs into pycircuit?

Could you explain this a bit more? The frontend would transform a Yosys JSON netlist?

dvc94ch · 2018-01-20T18:42:36Z

A cell in a yosys netlist has an attributes key with arbitrary key value pairs. I think that we can write a little script that looks for the attribute called 'electro-grammar' in each cell and extracts whatever information it can find and adds it as new attributes.

kasbah · 2018-01-20T18:48:39Z

Ah, neat, didn't know that. You'de probably still want the Python port to report errors to the user though?

I think it should probably be a separate package but it can live in this repo. Feel free to add it in js/lib for now. Later we can split it out into another package with yarn workspaces or something similar.

kasbah · 2018-01-20T19:18:44Z

Please ping me when you plan to take a larger break in working on this, I'll merge then. Please also make sure to push/pull-request any ongoing work so we have less risk of duplicating effort (as I will likely resume work on this too).

dvc94ch · 2018-01-20T19:24:41Z

We can also use it to pass stuff to netlistsvg. netlistsvg-component or something and use the cell name as a fallback if not specified.

There is also support for models in the netlist format:

The models are given as And-Inverter-Graphs (AIGs) in the following form:

    "models": {
      <model_name>: [
        /*   0 */ [ <node-spec> ],
        /*   1 */ [ <node-spec> ],
        /*   2 */ [ <node-spec> ],
        ...
      ],
      ...
    },

The following node-types may be used:

    [ "port", <portname>, <bitindex>, <out-list> ]
      - the value of the specified input port bit

    [ "nport", <portname>, <bitindex>, <out-list> ]
      - the inverted value of the specified input port bit

    [ "and", <node-index>, <node-index>, <out-list> ]
      - the ANDed value of the specified nodes

    [ "nand", <node-index>, <node-index>, <out-list> ]
      - the inverted ANDed value of the specified nodes

    [ "true", <out-list> ]
      - the constant value 1

    [ "false", <out-list> ]
      - the constant value 0

not sure yet if we can find a format that is simple and general enough that we can standardize on for analog circuit models.

Maybe something touchstone inspired would work [0] which is a list of n-port matrices linearized at multiple operating points.

Have you made any progress on your hdl?

[0] https://en.wikipedia.org/wiki/Touchstone_file

kasbah · 2018-01-20T19:35:52Z

Sounds good, yeah. Be sure to open up another issue/PR for it. My netlist HDL (called replicad) is what I got distracted by after I did the Antlr stubs for this.

kasbah · 2018-01-21T15:49:31Z

src/Units.g4

+
+temperature: NUMBER tunit tolerance?;
+tunit: KELVIN | 'u{00b0}'? CELSIUS;
+KELVIN:  K | K E L V I N;


I am wondering if there is any issues with having two lexer rules that look for k.

dvc94ch · 2018-01-21T16:06:03Z

Grammar doesn't work yet, but I think a human gets what it's supposed to do even if antlr doesn't. Will have to wait till tomorrow :/

kasbah · 2018-01-21T18:01:37Z

I am going to hack on this branch a bit tonight. Please make sure you have pushed everything so we don't get repeated work or conflicts.

kasbah · 2018-01-21T18:53:16Z

I merged everything except (wip) Experiment into the antlr branch and opened #4. I will continue working on that PR/branch.

kasbah · 2018-01-21T19:24:35Z

Actually, scratch that, I have merged up to and including 3a6b1d3 because after that you seem to have switched methodology, stopped testing and started experimenting, leaving behind the 2 previously passing tests.

It's fine to experiment of course but I don't want to merge in anything without tests at this stage. I think it's super easy to introduce regressions when expanding the grammar. If you want to have the work on transistors etc. merged I would suggest you wait till after the initial port and then include test cases in those PRs.

dvc94ch · 2018-01-21T19:40:29Z

Yep it's not finished, started adding tests in last commit

kasbah · 2018-01-21T19:47:58Z

Hmm, I can't really figure out how to continue work off of 3a6b1d3 without causing conflicts with all the rest of the work you did. Without tests I don't know if it's any good either though :). Also, you seem to be steering it steadily towards the pycircuit use-case (which makes sense, but we need to figure out how to resolve these two use-case directions we are seeing).

Do you mind just letting me do the initial Antlr port and then you can expand on top of that?

dvc94ch · 2018-01-21T20:20:20Z

Ok

dvc94ch · 2018-01-21T20:26:44Z

Yeah that is really a problem. I looked at the js testcases and realized that it seems like quite a lot of work that doesn't help pycircuit...

kasbah · 2018-01-21T20:33:31Z

Well, I don't mind doing the work because my initial use-case was search. I think there is still a massive overlap between the two uses, we are just highlighting the differences at the moment.

kasbah · 2018-01-23T14:43:58Z

python3/electro_grammar/electro_grammar.py

+        self.obj['dtype'] = 'led'
+
+    def exitSchottky(self, ctx):
+        self.obj['dtype'] = 'schottky'


Could we use more verbose names? diode_type instead of dtype. transistor_type instead of ttype.

kasbah · 2018-01-23T14:44:38Z

python3/tests/test_parser.py

+        d = parse('sk BAT54S')
+        assert d['type'] == 'diode'
+        assert d['dtype'] == 'schottky'
+        assert d['code'] == 'BAT54S'


What's the code? Is it the manufacturer part number?

yes, we can change it to mpn. the difference is only that for semiconductors the mpn has a structure, for arbitrary ic's it does not.

kasbah · 2018-01-23T15:08:42Z

Nice work! Does this have feature parity with the initial version now, in terms of recognizing capacitors, resistors and LEDs?

kasbah · 2018-01-23T15:30:52Z

Have you had any ambiguity warnings yet? We should check if the DiagnosticErrorListener is working if not.

kasbah · 2018-01-23T16:03:36Z

src/ElectroGrammar.g4


 WHITESPACE: [\p{White_Space}] -> skip;
+
+THROUGHHOLE: T H R O U G H '-'? H O L E -> skip;


Seems like you want a more lax parser ;)

Yeah just started hacking on the lax parser, commit is already dropped ;)

dvc94ch · 2018-01-23T16:35:32Z

I got some ambiguity warnings, but passing tests definitely swallow them, which isn't nice...

Once the lax parser starts ignoring unknown input we should be close to feature parity with the initial version.

dvc94ch · 2018-01-23T16:36:57Z

The current issue I'm trying to resolve is that it only recongizes a unit if it is followed by whitespace, so that Resistor is 'unlexable' instead of R 'es' i 'sto' r.

kasbah · 2018-01-23T16:43:16Z

I am going to merge up to and including 6de3e3c though there are still some minor issues (commented out code, console.logs and the output data schema). We can resolve those later.

Can you open new, separate pull requests for further fixes and features please? It will make it easier to keep up and review the changes.

kasbah · 2018-01-23T16:45:19Z

The current issue I'm trying to resolve is that it only recongizes a unit if it is followed by whitespace, so that Resistor is 'unlexable' instead of R 'es' i 'sto' r.

Hmmm, it might require a different approach. Maybe a sort of pre-lexer that splits things into words and then feeds them one at a time.

kasbah · 2018-01-23T17:07:36Z

@dvc94ch, are you sure you want to look at the lax parser right now? That's what I was going to work on...

dvc94ch · 2018-01-23T17:37:17Z

I created a new PR with my changes up to now. I'll start on something else...

dvc94ch requested a review from kasbah January 20, 2018 14:20

kasbah requested changes Jan 20, 2018

View reviewed changes

kasbah reviewed Jan 20, 2018

View reviewed changes

dvc94ch force-pushed the python3 branch from ac952c6 to 3e6bef1 Compare January 20, 2018 17:39

dvc94ch commented Jan 20, 2018

View reviewed changes

kasbah reviewed Jan 20, 2018

View reviewed changes

dvc94ch force-pushed the python3 branch 4 times, most recently from e48849e to 9262da0 Compare January 21, 2018 13:41

kasbah reviewed Jan 21, 2018

View reviewed changes

kasbah force-pushed the python3 branch from 8335175 to ad57abe Compare January 21, 2018 16:03

kasbah force-pushed the antlr branch from d875dc5 to 3a6b1d3 Compare January 21, 2018 19:15

kasbah and others added 13 commits January 22, 2018 19:58

Start a lax parser

cd25b63

Rename test_units to test_strict_units

1c02e00

Add "prettier" code formatter

520f768

Add ambiguity diagnostic output to index.js

bd86c26

Start ignorer error listeners in lax_parser

006dd32

python3: Add ambiguity diagnostics

d03cc1b

js: set ambiguity output directly like in python

eea48b4

python3: Add unit parsing tests.

da2febf

python3: Add tolerance test case.

3d9e6e3

python3: Add package size tests.

3a467c0

python3: Add dielectric tests.

312f9c5

python3: Add passive tests.

af3bf78

python3: Add semiconductor tests.

6feeebf

dvc94ch force-pushed the python3 branch from 3b01938 to 6feeebf Compare January 22, 2018 18:59

WIP: js port.

65341af

kasbah reviewed Jan 23, 2018

View reviewed changes

dvc94ch added 2 commits January 23, 2018 16:03

js: Feature parity with python port.

d663ca3

antlr: Add some missing unit specifiers.

6de3e3c

Fix prepare cpl.

e6fcb43

kasbah reviewed Jan 23, 2018

View reviewed changes

kasbah closed this Jan 23, 2018


		capacitance : NUMBER CPREFIX FARAD?;
		electro_grammar : capacitor;


		WHITESPACE: [\p{White_Space}] -> skip;

		THROUGHHOLE: T H R O U G H '-'? H O L E -> skip;

Add python3 backend. #1

Add python3 backend. #1

Conversation

dvc94ch commented Jan 20, 2018

kasbah commented Jan 20, 2018

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

dvc94ch commented Jan 20, 2018

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

kasbah Jan 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasbah Jan 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasbah commented Jan 20, 2018

dvc94ch commented Jan 20, 2018

kasbah commented Jan 20, 2018 • edited Loading

kasbah commented Jan 20, 2018

dvc94ch commented Jan 20, 2018

kasbah commented Jan 20, 2018 • edited Loading

Choose a reason for hiding this comment

dvc94ch commented Jan 21, 2018

kasbah commented Jan 21, 2018

kasbah commented Jan 21, 2018 • edited Loading

kasbah commented Jan 21, 2018

dvc94ch commented Jan 21, 2018

kasbah commented Jan 21, 2018

dvc94ch commented Jan 21, 2018

dvc94ch commented Jan 21, 2018

kasbah commented Jan 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasbah commented Jan 23, 2018 • edited Loading

kasbah commented Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvc94ch commented Jan 23, 2018

dvc94ch commented Jan 23, 2018

kasbah commented Jan 23, 2018

kasbah commented Jan 23, 2018

kasbah commented Jan 23, 2018

dvc94ch commented Jan 23, 2018

kasbah Jan 20, 2018 •

edited

Loading

kasbah Jan 20, 2018 •

edited

Loading

kasbah commented Jan 20, 2018 •

edited

Loading

kasbah commented Jan 20, 2018 •

edited

Loading

kasbah commented Jan 21, 2018 •

edited

Loading

kasbah commented Jan 23, 2018 •

edited

Loading

kasbah commented Jan 23, 2018 •

edited

Loading