ceedee666 / opensap_python_intro

This repository contains several Jupyter notebooks. These notebooks are the course material for an upcoming openSAP course on Python 🐍.
Creative Commons Zero v1.0 Universal
5 stars 1 forks source link

Check Exercises Week 5 #141

Closed stjaco62 closed 2 years ago

stjaco62 commented 2 years ago

We should check all exercises of week 5 for consistency. To start the list, I would propose to check the following:

Tests: Are the feedback messages understandable and consistent, e. g. when referring to functions, we should use "[...] number of print() statements" instead of "[...] print statements" Check for typos, especially in the tests Are the Attributes in CodeOcean set correctly (Public and activated Auto-Complete)

More to add to the list?

ceedee666 commented 2 years ago

It is important to also try some of the edge cases that users might do. For example:

GlennVerhaag commented 2 years ago

I worked through the exercises of week 5, they all worked as intended and I was able to get 100% on all of them (might be a first 😄)

The only thing I noticed, was that the assignment exercise was very difficult this week. For me, the main obstacle was fully understanding the task at hand.

image

The illustration above was helpful, however I think it could be restructured a bit. For example, structuring it like this:

  1. Clear Text p y t h o n i s b e a u t i f u l This is the text input by the user, transformed into lower case and spaced apart to represent the individual characters.
  2. Keyword r a n d o m r a n d o m r a n d o m r This is the keyword input by the user, repeated to match the length of the input text (including spaces)

etc.

would help learners understand the Vignère Cipher and the individual steps faster.

simonfreud commented 2 years ago

Week 5 assignment: One functionality test is failing and is giving me "output = None" as a reason, but that shouldn't be the case. I did try it myself with the inputs that allegedly failed but I think they should be correct. The linker is failing aswell caused by multiple inconsistent return values but I only have one return value in each function. image image

ceedee666 commented 2 years ago

@simonfreud you code is not correct. The function encrypt_text() should return the encryptet text, not print it. Please check if this is clear in the description.

simonfreud commented 2 years ago

@ceedee666 You are right and the description is correct. This mistake was on my side. Now I'm receiving a new error. image

ceedee666 commented 2 years ago

your code is still not correct. encrypt_letter and calculate_shift should return a value even is there is no encryption happening (as for special chracters).

I'll have a look at the tests to add a better error message.

simonfreud commented 2 years ago

I changed this aswell but that did not clear the error. image

ceedee666 commented 2 years ago

@Hiltiprant I updated the tests an the description (important) to be able to provide sensible error messages. Coudl you please update CodeOcean? @simonfreud @GlennVerhaag @kaisdeljam @LauritsE98 can you test afterwards again. Be aware that the description was changed.

simonfreud commented 2 years ago

All exercises checked.

Hiltiprant commented 2 years ago

@Hiltiprant I updated the tests an the description (important) to be able to provide sensible error messages. Coudl you please update CodeOcean? @simonfreud @GlennVerhaag @kaisdeljam @LauritsE98 can you test afterwards again. Be aware that the description was changed.

Everything in CO updated.

@ceedee666 Did you already update the description to clarify things like asked here?

simonfreud commented 2 years ago

One test is still failing with my solution.

Hiltiprant commented 2 years ago

One test is still failing with my solution.

I think, more information would be helpful... Or still the same error message?

simonfreud commented 2 years ago

Still the same

I changed this aswell but that did not clear the error. image

Hiltiprant commented 2 years ago

@ceedee666 With Simons Code

import string

def encrypt_letter(letter, shift):
    if letter in string.ascii_lowercase:
        ind = string.ascii_lowercase.index(letter)
        ind = (ind + shift) % 26
        secret_letter = string.ascii_lowercase[ind]
    else:
        secret_letter = letter
    return secret_letter

def calculate_shifts(letter):
    ind = string.ascii_lowercase.index(letter)
    return ind

def encrypt_text(text, keyword):
    text = text.lower()
    keyword = keyword.lower()

    encrypted_text = ""

    for index, letter in enumerate(text):
        key_letter = keyword[index % len(keyword)]
        shift = calculate_shifts(key_letter)
        if letter.isalpha():
            encrypted_text += encrypt_letter(letter, shift)
        else:
            encrypted_text += letter
    return encrypted_text

text = input("Which text should be encrypted: ")
keyword = input("Which keyword should be used? ")

print(encrypt_text(text, keyword))

the functional_tests.py fail at https://github.com/ceedee666/opensap_python_intro/blob/21a6e1161cbdab56c4eb383aa0160341dfb4acd4/exercises/week_5/assignment/functional_tests.py#L143

I don't really understand why this happens. Is this something we can fix or is this an issue with his code that is not 100 % correct or s.th. similar?

kaisdeljam commented 2 years ago

Exercises checked. @simonfreud code seems to work now.

Don't forget to change the task body. It looks awful. This part in CO: image

and in openSAP it should also be changed image

stjaco62 commented 2 years ago

Hi,

the .md-format is broken both in CO and openSAP. Is it possible, to use a picture instead? An HTML-Table is probably too complicated. Any proposal?

@susanne: Can we include an image in the opensap page?

regards

stephan

Von: kaisdeljam @.> Gesendet: Donnerstag, 5. Mai 2022 16:17 An: ceedee666/opensap_python_intro @.> Cc: Jacobs, Stephan @.>; Assign @.> Betreff: Re: [ceedee666/opensap_python_intro] Check Exercises Week 5 (Issue #141)

Exercises checked. @simonfreud https://github.com/simonfreud code seems to work now.

Don't forget to change the task body. It looks awful. This part in CO: https://user-images.githubusercontent.com/102662808/166943248-710a37c6-9db3-41cd-b71a-4727aac4b1a8.png

and in openSAP it should also be changed https://user-images.githubusercontent.com/102662808/166943337-dfc509b1-6ccf-4e45-a254-a44391f6fb27.png

— Reply to this email directly, view it on GitHub https://github.com/ceedee666/opensap_python_intro/issues/141#issuecomment-1118610710 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AGLXZO6XZPN6YTCSUFNJJKLVIPJ65ANCNFSM5S4MA64Q . You are receiving this because you were assigned. https://github.com/notifications/beacon/AGLXZO6YFESZX7PPXAKDPL3VIPJ65A5CNFSM5S4MA642YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOIKWKKFQ.gif Message ID: @. @.> >

simonfreud commented 2 years ago

This is not my code. My code is still not passing all tests. image The solution @Hiltiprant posted is passing all tests but it is working with string import. I will look into my code again and maybe I can find anything that could be the reason for these tests to fail, but I'm pretty confident that it should work.

ceedee666 commented 2 years ago

@simonfreud please post you code as text here and I‘ll check. The reason seems to be that in certain cases your code does not return a character/string and therefore the concatenation in your code fails.

SusanneBusemann commented 2 years ago

Hi, the .md-format is broken both in CO and openSAP. Is it possible, to use a picture instead? An HTML-Table is probably too complicated. Any proposal? @Susanne: Can we include an image in the opensap page? regards stephan Von: kaisdeljam @.> Gesendet: Donnerstag, 5. Mai 2022 16:17 An: ceedee666/opensap_python_intro @.> Cc: Jacobs, Stephan @.>; Assign @.> Betreff: Re: [ceedee666/opensap_python_intro] Check Exercises Week 5 (Issue #141) Exercises checked. @simonfreud https://github.com/simonfreud code seems to work now. Don't forget to change the task body. It looks awful. This part in CO: https://user-images.githubusercontent.com/102662808/166943248-710a37c6-9db3-41cd-b71a-4727aac4b1a8.png and in openSAP it should also be changed https://user-images.githubusercontent.com/102662808/166943337-dfc509b1-6ccf-4e45-a254-a44391f6fb27.png — Reply to this email directly, view it on GitHub <#141 (comment)> , or unsubscribe https://github.com/notifications/unsubscribe-auth/AGLXZO6XZPN6YTCSUFNJJKLVIPJ65ANCNFSM5S4MA64Q . You are receiving this because you were assigned. https://github.com/notifications/beacon/AGLXZO6YFESZX7PPXAKDPL3VIPJ65A5CNFSM5S4MA642YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOIKWKKFQ.gif Message ID: @. @.> >

Sure we can include images on text pages, see here: https://open.sap.com/courses/python1/items/7vKdMrX2ZI1kDftCiC40at Just upload the image (@Hiltiprant maybe you can provide advice regarding the size) in sharepoint, we´ll upload and provide the link which can be used in CO as well as on the openSAP page.

simonfreud commented 2 years ago

@ceedee666 I have rewatched my code but I have no clue at all why it doesn't wanna work.

abc = "abcdefghijklmnopqrstuvwxyz"

def encrypt_letter(char, key):
    if char.isalpha() is True:
        for i in range(len(abc)):
            if abc[i] == char:
                return abc[(i + key) % 26]
    else:
        return char

def calculate_shifts(char):
    if char.isalpha() is True:
        for i in range(len(abc)):
            if abc[i] == char:
                return i
    else:
        return 0

def encrypt_text(txt, key):
    output = ""
    for i in range(len(txt)):
        output += encrypt_letter(txt[i], calculate_shifts(key[i % len(key)]))
    return output

text = input("Which text should be encrypted: ")
text = text.lower()
keyword = input("Which keyword should be used? ")
keyword = keyword.lower()

print(encrypt_text(text, keyword))
ceedee666 commented 2 years ago

I just update the description of the exercise. IMO the description was fostering bad programming style. The description stated:

Before you pass these strings to encrypt_text() turn all letters into their lower equivalents using .lower()

This is bad style and very error prone. A function should never depend on required precondition beeing set outside it scope. Therefore, I updated the description to:

Get both the clear text and the keyword by the input() function. The encrypt_text() function should turn the text and the keyword into lower case equivalents using .lower(). Finally, print the return value from encrypt_text().

@Hiltiprant cloud you please update CodeOcean @simonfreud @stjaco62 @GlennVerhaag @kaisdeljam: Could you afterwards please recheck the exercise.

ceedee666 commented 2 years ago

@simonfreud if you change this code

def encrypt_text(txt, key):
    output = ""
    for i in range(len(txt)):
        output += encrypt_letter(txt[i], calculate_shifts(key[i % len(key)]))
    return output

text = input("Which text should be encrypted: ")
text = text.lower()
keyword = input("Which keyword should be used? ")
keyword = keyword.lower()

to

def encrypt_text(txt, key):
    txt = txt.lower()
    key = key.lower()
    output = ""
    for i in range(len(txt)):
        output += encrypt_letter(txt[i], calculate_shifts(key[i % len(key)]))
    return output

text = input("Which text should be encrypted: ")
keyword = input("Which keyword should be used? ")

your solution should pass all the test.

Hiltiprant commented 2 years ago

Exercises checked. @simonfreud code seems to work now.

Don't forget to change the task body. It looks awful. This part in CO: image

and in openSAP it should also be changed image

Hi, the .md-format is broken both in CO and openSAP. Is it possible, to use a picture instead? An HTML-Table is probably too complicated. Any proposal? @Susanne: Can we include an image in the opensap page? regards stephan

@stjaco62 You are referring to the same table @kaisdeljam also mentioned, right? I will contact Sebastian, if we can create a table with html. My attempts (online conversion md table --> html table) have failed so far. Otherwise we can still use an image. Regarding the images like @SusanneBusemann mentioned: As big as possible. We can do scaling afterwards in CO like I did here, for example, e.g.:

[1]: https://s3.xopic.de/opensap-public/courses/2qRB6Gz3FcfD2OBbnSCf8m/rtfiles/4kJjPFWReIxSEG4dZdkCzo/openSAP_python1_Week_1_BO_formula_1.png
{: height="24px"}

This is not my code. My code is still not passing all tests. image The solution @Hiltiprant posted is passing all tests but it is working with string import. I will look into my code again and maybe I can find anything that could be the reason for these tests to fail, but I'm pretty confident that it should work.

Sorry for that, oviously I pasted the wrong code here. But I tested it with your code (taken from CO)

I updated the exercise description in CO, but I will update it once more when we have a solution for the table format.

simonfreud commented 2 years ago

@ceedee666 This solved my problem. All tests are passing now!

Hiltiprant commented 2 years ago

Mal kurz auf Deutsch, für Englisch habe ich gerade keine Zeit. Ich habe Sebastian kontaktiert und er bringt voraussichtlich Sonntag eine Änderung auf CO, die das Nutzen von HTML-Tabellen ermöglicht. Sähe dann wohl so aus und damit meiner Meinung nach super:

Tabelle

Als Backup hätte ich trotzdem gerne ein Bild für den unwahrscheinlichen Fall, dass was schief geht (bei Sebastian meiner Erfahrung nach aber eigentlich nicht zu erwarten 😉). Bilder einbinden klappt nämlich auf jeden Fall.

Kann jemand aus dem MD eine Tabelle als Bild (Screenshot) erstellen (möglichst hohe Auflösung, also Fenster-Skalierung hochsetzen), im Sharepoint ablegen und @SusanneBusemann darüber Bescheid geben? Die leitet das dann weiter und wir kriegen einen Link zum Einbinden (kann ich dann machen). Muss kein md sein, kann auch beispielsweise in Excel sein, Hauptsache es sieht schön aus, z. B. @stjaco62 aka Excel-man?

simonfreud commented 2 years ago

@Hiltiprant Ich kann dir gerne eine schöne Exel machen!

Hiltiprant commented 2 years ago

@Hiltiprant Ich kann dir gerne eine schöne Exel machen!

Perfekt! Dann einfach davon einen Screenshot machen und im Sharepoint hochladen (oder an Susanne schicken, falls kein Zugriff) mit dem Hinweis einen Link zu generieren zum Einbinden. Wie gesagt - vermutlich (hoffentlich) brauchen wir es nicht, aber zur Sicherheit...

kaisdeljam commented 2 years ago

@Hiltiprant "für Englisch habe ich gerade keine Zeit" - super😂

stjaco62 commented 2 years ago

Hallo, habe einen Screenshot der Tabelle im Anhang. Ich hoffe das ist okay. Wenn nicht, bitte nochmal melden. Das Bild liegt auch auf github table .

simonfreud commented 2 years ago

@Hiltiprant Done! Week 5 Assignment Tabelle JPG

Hiltiprant commented 2 years ago

@stjaco62 @simonfreud Nice, thanks! @SusanneBusemann Can you pass the images to your colleagues to create the xopic-link from here or do we need to upload the images to Sharepoint?

Hiltiprant commented 2 years ago

I just fixed some typos in the assignment in 9b033a0c21b7433dc4543362b685fbcc70cd2c54. I already updated the files in CO, but there were some typos in the notebooks, too. What needs to be done with that @stjaco62 @SusanneBusemann ?

SusanneBusemann commented 2 years ago

@Hiltiprant I let you know (here) as soon as I have the link for the table, upload is requested. For the notebooks I would replace them with the corrected ones but would like to discuss that briefly on Monday.

Hiltiprant commented 2 years ago

@all Sebastian enabled rendering of markdown tables on CO, so the screenshots of tables are not necessary, you can have a look, I think this looks very nice now. Thanks anyways and have a nice weekend!

SusanneBusemann commented 2 years ago

@Hiltiprant I received 2 files for this table, a smaller and a bigger one; they are both uploaded and here are the links (so you can choose which one fits better):

https://s3.xopic.de/opensap-public/courses/2qRB6Gz3FcfD2OBbnSCf8m/rtfiles/7eGIYKPpeyL7G50N4GtcdY/table.png

https://s3.xopic.de/opensap-public/courses/2qRB6Gz3FcfD2OBbnSCf8m/rtfiles/1sEO5flWHvq9DjEHMmjsUT/Week_5_Assignment_Tabelle_JPG.jpg

SusanneBusemann commented 2 years ago

@ALL Sebastian enabled rendering of markdown tables on CO, so the screenshots of tables are not necessary, you can have a look, I think this looks very nice now. Thanks anyways and have a nice weekend!

Oh, I just saw this - well, then see the links as a special gift and keep them for any backup reasons you might have ;-)

I´ll work on the description on the openSAP page right now and had one remark - the very last line "gygkcz if fqrlyb nvahwwrll" --> is this intended?

SusanneBusemann commented 2 years ago

Okay, I adjusted the text on the page: https://open.sap.com/courses/python1/items/PgDEeKkb13BPhxs9qmPD7

The rendering of markdown tables doesn´t seem to work on openSAP, therefore I used the screenshot. I think it looks fine, what do you think? And the last line I guess has something to do with the secret text? Just let me know if there is anything I should edit.

Hiltiprant commented 2 years ago

@SusanneBusemann The description on openSAP looks fine. The last line is correct, as it is an example for the encrypted text.

SusanneBusemann commented 2 years ago

@ALL Are all exercises now OK, including Bonus? As I did not read anything about that one I wasn´t sure, so just to ensure that everything is working fine before go-live tomorrow.

simonfreud commented 2 years ago

@kaisdeljam @GlennVerhaag and I checked all exercises and @Hiltiprant @ceedee666 @stjaco62 solved all problems we ran into.

stjaco62 commented 2 years ago

@ceedee666 Re-worked my solution due to your above comment "I just update the description of the exercise. IMO the description ..." Now my solution again gets all points (however I have to redo the explanation videos.)

One more issue: The linter only assigns 9.17 out of 10 as I followed the common pattern described in the last paragraph of week5_unit5. That means, the argument name and the parameter name are both the same. Which should not be according to the linter.

stjaco62 commented 2 years ago

Closed as week 5 is finished