Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JavaScript backend core functionality #159

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

hmartiro
Copy link
Contributor

@hmartiro hmartiro commented Jun 3, 2022

Adds a javascript codegen backend to symforce. This change supports 1D and 2D matrices as javascript Array and Array of Arrays. It generates functions that run in node. It does not yet support geo, cam, or struct types.

Copy link
Member

@aaron-skydio aaron-skydio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

High-level looks nice, will hold off on full review until it's more complete (looks based on util.jinja like it's not quite ready yet?)

@hayk-skydio
Copy link
Contributor

@aaron-skydio this is ready for a pass now

@hayk-skydio hayk-skydio requested a review from aaron-skydio June 5, 2022 14:39
symforce/codegen/backends/cpp/cpp_config.py Show resolved Hide resolved
doc_comment_line_prefix: str = " * "
line_length: int = 100
use_eigen_types: bool = True
# NOTE(hayk): Add JS autoformatter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably prettier? Which unfortunately doesn't seem to be on PyPI

behavior for codegen compatibility and efficiency.
"""

def __init__(self, settings: T.Dict[str, T.Any] = None) -> None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

T.Mapping instead of T.Dict

"""

def __init__(self, settings: T.Dict[str, T.Any] = None) -> None:
settings = dict(settings or {},)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No trailing comma?

Comment on lines 48 to 51
elif extension == "ts":
return FileType.TYPESCRIPT
elif extension == "js":
return FileType.JAVASCRIPT
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably leave typescript in here, unless we also delete it from the enum above?

)

def test_javascript_codegen(self) -> None:
for config in (codegen.PythonConfig(), codegen.CppConfig(), codegen.JavascriptConfig()):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the reasoning for generating it for all three just for debug purposes?

@@ -0,0 +1,52 @@
// -----------------------------------------------------------------------------
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably go in symforce_function_codegen_test_data? And use the pattern in the other codegen tests so it automatically generates on both backends

@hmartiro hmartiro force-pushed the centralize-backend-specific-code branch 2 times, most recently from 3715c7d to 5874834 Compare June 11, 2022 20:17
@hayk-skydio hayk-skydio changed the base branch from centralize-backend-specific-code to main July 1, 2022 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants