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

Fix exit statement in Fortran with Intel compiler #1624

Conversation

harsha-mangena
Copy link
Contributor

@harsha-mangena harsha-mangena commented Nov 18, 2023

The Intel compiler does not allow specifying the precision in a stop statement. To fix this, a condition was added in _print_SysExit to print LiteralInteger objects with default precision. Fixes #1554.

- added a check for epression if it is `LiteralInteger` or not
- no type specifier is added for NativeInteger
@pyccel-bot
Copy link

pyccel-bot bot commented Nov 18, 2023

Hello! Welcome to Pyccel! Thank you very much for your contribution ❤️.

I am the GitHub bot. I will help guide you through the different stages necessary to validate a review in Pyccel. If you haven't yet seen our developer docs make sure you check them out here. Amongst other things they describe the review process that we have just started. You can also get in touch with our other developers on our Pyccel Discord Server.

To begin with I will give you a short checklist to make sure your pull request is complete. Please tick items off when you have completed them or determined that they are not necessary for this pull request. If you want me to run any specific tests to check out corner cases that you can't easily check on your computer, you can request this using the command /bot run X. Use the command /bot show tests to see a full list of the tests I can run. Once you have finished preparing your pull request and are ready to request reviews just take your PR out of draft, or let me know with the command /bot mark as ready. I will then run the full suite of tests to check that everything is as neat as you think before asking other contributors for reviews. Tests will not run automatically before this point to avoid wasting resources. You can get a full list of commands that I understand using /bot commands.

Please begin by requesting your checklist using the command /bot checklist

@github-actions github-actions bot marked this pull request as draft November 18, 2023 06:13
@pyccel-bot
Copy link

pyccel-bot bot commented Nov 18, 2023

@yguclu, @EmilyBourne, @bauom, please can you check if I can trust this user. If you are happy, let me know with /bot trust user

@pyccel-bot
Copy link

pyccel-bot bot commented Nov 18, 2023

This bot reacts to all comments which begin with /bot. This phrase can be followed by any of these commands:

  • show tests : Lists the tests which can be triggered
  • run X : Triggers the test X (acceptable values for X can be seen using show tests). Multiple tests can be specified separated by spaces.
  • try V X : Triggers the test X (acceptable values for X can be seen using show tests) using Python version V. Multiple tests can be specified separated by spaces, but all will use the same Python version.
  • mark as ready : Runs the PR tests. If they pass then it adds the appropriate review flag and requests reviews. This command should be used when the PR is first ready for review, or when a review has been answered.
  • commands : Shows this list detailing all the commands which are understood.
  • trust user X : Tells the bot that a new user X is trusted to run workflows (prevents misuse of GitHub actions for mining etc). This command can only be used by a trusted reviewer.

Beware: if you have never contributed to this repository and you are not a member of the Pyccel organisation, the bot will ignore all requests to run tests until permitted by a trusted reviewer.

@EmilyBourne
Copy link
Member

/bot trust user harsha-mangena

@pyccel-bot
Copy link

pyccel-bot bot commented Nov 18, 2023

@harsha-mangena It looks like the Pyccel maintainers trust you 🚀. I will now start running tests when you request them.

@EmilyBourne
Copy link
Member

Hi @harsha-mangena , are you still working on this PR? Do you need a hand or want me to complete it for you?

@harsha-mangena
Copy link
Contributor Author

Hi @EmilyBourne, I couldn't put time on this as its my end semester week, Will work on this on this weekend.
It would be helpful for me If you can guide me through this.

@EmilyBourne
Copy link
Member

/bot run linux

@EmilyBourne
Copy link
Member

@harsha-mangena No problem. Great news if you will have time this weekend :)
I don't think there's too much left to do. You need to:

  1. Interact with the bot to get the checklist of jobs to complete the PR
    As described by the bot the method for doing this is to leave a comment containing /bot checklist.
    The bot will then modify this comment so it contains a checklist where you can tick things off.
    It's mostly basic things which are easily forgotten (e.g. CHANGELOG)
  2. Fix the bugs. As I hinted in my last comment on your code the current implementation doesn't quite work. For example:
    • the test tests/pyccel/scripts/exits/positive_exit3.py is failing with:
          /home/runner/work/pyccel/pyccel/tests/pyccel/scripts/exits/__pyccel__/__pyccel__gw0/prog_positive_exit3.f90:11:6:
      
         11 |   stop exit_code
            |      1
      Error: STOP code at (1) must be default integer KIND=4
      
      To fix this, non-literals need to be converted to precision=4 (as was done in the original code)
    • The test tests/pyccel/scripts/exits/negative_exit2.py is failing with:
      home/runner/work/pyccel/pyccel/tests/pyccel/scripts/exits/__pyccel__/__pyccel__gw0/prog_negative_exit2.f90:8:6:
              8 |   stop -Literal(345)
                |      1
      Error: STOP code at (1) must be either INTEGER or CHARACTER type
      
      To fix this you need to make sure you are using the _print function whenever you want to print an instance of a class. (For LiteralInteger you probably need expr.status.python_value to get the underlying int)
    • You can find the list of relevant tests here:
      @pytest.mark.parametrize( "test_file", ["scripts/exits/empty_exit.py",
      "scripts/exits/negative_exit1.py",
      "scripts/exits/negative_exit2.py",
      "scripts/exits/positive_exit1.py",
      "scripts/exits/positive_exit2.py",
      "scripts/exits/positive_exit3.py",
      "scripts/exits/zero_exit.py",
      ] )

      If you have managed to install pyccel then you should be able to run the command line tool on these files to see the compiler errors.

@harsha-mangena
Copy link
Contributor Author

@EmilyBourne,
I regret the delayed response, I had tried this in several ways but I don't know why the code isn't working and test cases as well. So I kindly request if this issue is in priority, Please assign this to someone.

@EmilyBourne
Copy link
Member

@harsha-mangena no problem for the slow reply, especially over the holidays!
This is not a priority issue. As far as I am aware there are very few people using exit statements and even fewer using intel. So if you want to keep going then feel free. I'm not quite sure where you're stuck though. I think between your first version and this version you have at some point implemented working code for each section.
If you have more specific questions then you're welcome to contact me on discord. But if you'd rather leave it then I'm happy to take over. Whatever you prefer

@EmilyBourne
Copy link
Member

/bot run linux intel

@EmilyBourne
Copy link
Member

/bot run intel

@EmilyBourne
Copy link
Member

/bot run intel

@EmilyBourne
Copy link
Member

EmilyBourne commented Jan 3, 2024

Here is your checklist. Please tick items off when you have completed them or determined that they are not necessary for this pull request:

  • Write a clear PR description
  • Add tests to check your code works as expected
  • Update documentation if necessary
  • Update Changelog
  • Ensure any relevant issues are linked
  • Ensure new tests are passing

@EmilyBourne EmilyBourne marked this pull request as ready for review January 3, 2024 10:34
Copy link

@pyccel-bot pyccel-bot bot left a comment

Choose a reason for hiding this comment

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

There seems to be lines in this PR which aren't tested. Please take a look at my comments and add tests which cover the new code.

If this is modified code which cannot be easily tested in this PR please open an issue to request that this code be either removed or tested. Once you have done that please leave a message on the relevant conversation beginning with the line /bot accept and referencing the issue.

Similarly if the new code cannot be tested for some reason, please leave a comment beginning with the line /bot accept on the relevant conversation explaining why the code can't be tested.

pyccel/codegen/printing/fcode.py Show resolved Hide resolved
@github-actions github-actions bot marked this pull request as draft January 3, 2024 10:49
@pyccel-bot
Copy link

pyccel-bot bot commented Jan 3, 2024

Unfortunately your PR is not passing the tests so it is not quite ready for review yet. Let me know when it is fixed with /bot mark as ready.

@EmilyBourne
Copy link
Member

/bot run coverage

Copy link

@pyccel-bot pyccel-bot bot left a comment

Choose a reason for hiding this comment

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

Good job ! Your PR is using all the code it added/changed.

@EmilyBourne EmilyBourne marked this pull request as ready for review January 4, 2024 17:02
@pyccel-bot pyccel-bot bot added the Ready_to_merge Approved by senior developer. Ready for final approval and merge label Jan 4, 2024
@yguclu yguclu changed the title [Bug]Cannot specify precision in stop statement Fix exit statement in Fortran with Intel compiler Jan 8, 2024
@yguclu yguclu merged commit b50a87d into pyccel:devel Jan 8, 2024
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready_to_merge Approved by senior developer. Ready for final approval and merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot specify precision in stop statement
3 participants