Skip to content

Feat spectra sim#7

Open
jiaqingxie wants to merge 9 commits intoInternScience:mainfrom
jiaqingxie:feat-spectra-sim
Open

Feat spectra sim#7
jiaqingxie wants to merge 9 commits intoInternScience:mainfrom
jiaqingxie:feat-spectra-sim

Conversation

@jiaqingxie
Copy link
Copy Markdown
Contributor

@jiaqingxie jiaqingxie commented Mar 31, 2026

Summary

Closes: #(issue)

References

If this PR adds a new skill

  • The new skill folder is under skills/<skill-name>/
  • SKILL.md frontmatter includes name and description
  • name matches folder name and uses kebab-case
  • I checked skills/README.md and assigned this skill to the correct category
  • I considered whether this should be merged into an existing skill instead of creating a new one
  • I updated skills/README.md (name, description, status, and link if applicable)

Chosen category in skills/README.md:

Merge/Separate decision note:

Validation

# Example:
# npx skills add . --list
# npx skills add . --skill <skill-name>

Impact

  • Affected skills:
  • Backward compatibility:
  • Risks / limitations:

Checklist

  • Change type: New skill / Update skill / Bug fix / Docs / Other
  • I read CONTRIBUTING.md
  • I searched for duplicate PRs/issues
  • I tested changes locally
  • I updated related docs (README / skills catalog / examples) if needed

xiejiaqing and others added 9 commits March 25, 2026 13:47
… vendor fiora under assets; fix ms submodule

Made-with: Cursor
Replace the vendored local fioRa runtime with a browser-driven online workflow so the MS skill stays lightweight while still exporting MSP, MGF, and PNG outputs.

Made-with: Cursor
Remove duplicated NMRNet sources and demo data from the bundled skill while keeping the runtime dictionary and Uni-Core setup intact, reducing the repository footprint without changing the inference workflow.

Made-with: Cursor
Add self-contained 31P and 19F NMR skills that predict shifts from CIF crystal structures using shared NMRNet assets, so the new workflows can be run directly from the skills directory.

Made-with: Cursor
Update the skills catalog so the new P and F NMR simulation skills are listed with folder links, descriptions, and completed status.

Made-with: Cursor
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces two new skills, f-nmr-simulation and p-nmr-simulation, for predicting solid-state 19F and 31P NMR chemical shifts using the NMRNet model. Additionally, the ms-spectra-simulation skill has been refactored to utilize the fioRa online application via Playwright browser automation, replacing the previous local installation. Review feedback suggests optimizing the download of large model weights to avoid potential memory issues and using the csv module for safer payload construction in the mass spectrometry simulation script.

I am having trouble creating individual review comments. Click here to see my feedback.

skills/f-nmr-simulation/f_nmr_simulation.py (57)

medium

Using zf.read(zip_path) loads the entire file into memory. Since the model weights are approximately 560 MB, this can lead to Out-Of-Memory (OOM) errors in environments with limited RAM. It is more efficient to stream the file content directly to disk using shutil.copyfileobj.

            import shutil
            with zf.open(zip_path) as source, open(local_path, "wb") as target:
                shutil.copyfileobj(source, target)

skills/p-nmr-simulation/p_nmr_simulation.py (57)

medium

Using zf.read(zip_path) loads the entire file into memory. Since the model weights are approximately 560 MB, this can lead to Out-Of-Memory (OOM) errors in environments with limited RAM. It is more efficient to stream the file content directly to disk using shutil.copyfileobj.

            import shutil
            with zf.open(zip_path) as source, open(local_path, "wb") as target:
                shutil.copyfileobj(source, target)

skills/ms-spectra-simulation/ms_spectra_simulation.py (70-74)

medium

The CSV payload for the fioRa app is constructed using f-strings without escaping. If args.name or args.smiles contains commas or newlines, the resulting CSV will be malformed. Using the csv module ensures that fields are correctly quoted and escaped.

def build_textbox_payload(args: argparse.Namespace) -> str:
    import io
    import csv
    output = io.StringIO()
    writer = csv.writer(output, lineterminator="\n")
    writer.writerow(["Name", "SMILES", "Precursor_type", "CE", "Instrument_type"])
    writer.writerow([args.name, args.smiles, args.precursor, f"{args.ce:g}", args.instrument])
    return output.getvalue()

@little1d
Copy link
Copy Markdown
Collaborator

little1d commented Apr 1, 2026

@jiaqingxie 可能需要对应修改一下 skills/README.md,把这两个新的 skill 描述加上,其他没什么问题

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.

2 participants