Move numeric_types.py from fud/verilator to calyx-py (another attempt at #1715 )
#1719
Conversation
| @@ -8,9 +8,8 @@ | |||
| ) | |||
| from calyx.utils import float_to_fixed_point | |||
| from math import factorial, log2 | |||
| from fud.stages.verilator import numeric_types | |||
| from calyx.numeric_types import FixedPoint | |||
There was a problem hiding this comment.
It is a great outcome here that the calyx-py library doesn't need to depend on fud for this. That direction of dependency was always extremely weird.
| @@ -3,8 +3,7 @@ | |||
| from pathlib import Path | |||
| import simplejson as sjson | |||
| import numpy as np | |||
| from fud.stages.verilator.numeric_types import FixedPoint, Bitnum | |||
| from fud.errors import InvalidNumericType | |||
| from calyx.numeric_types import FixedPoint, Bitnum, InvalidNumericType | |||
There was a problem hiding this comment.
It could be described as slightly weird, although not overwhelmingly weird, that fud depends on calyx-py now. Like, if you think of the calyx-py library as being for generating code, it's not clear why fud would need that. But actually, calyx-py currently is sort of sprawling and covers lots of things, including all our individual code generators (it is not just a code generation library). Therefore this doesn't seem like much messier than it was before, so I think it's fine!
There was a problem hiding this comment.
Hm, I think we should eventually externalize all these stages from the core logic in fud so that these false dependencies can be eliminated. The fud API is rich enough that we don't actually need to have these stages sitting in the definition of fud itself anymore.
There was a problem hiding this comment.
Is the idea to have all stages be external then? Or rather to get rid of the notion of non-external stages?
There was a problem hiding this comment.
Yeah, I'd say barring any API problems, there should be no internal stages whatsoever.
|
@calebmkim Anything blocking us from merging this? |
|
Docker fails when I tried to push this. |
|
@calebmkim let's try to get this merged? Hopefully the new docker image stuff allows this to be tested correctly? |
|
Ok, I'll try to merge (if it still doesn't work I can revert the change). |
…ttempt at #1715 ) (#1719) * refactor numeric types * moved test * changed error location * fix test location --------- Co-authored-by: Rachit Nigam <[email protected]>
This is another attempt at #1715. I merged #1715 without realizing that it would mess up Docker (see this error).
Therefore, I reverted the merge. I have reopened another PR that does the exact same thing as #1715. I would like to merge it, but first have to figure out why the Docker stuff was failing.
Apologies for this being so confusing.